Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support sciml blue and yas styleguides #440

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lucaferranti
Copy link
Contributor

@lucaferranti lucaferranti commented Sep 5, 2024

PR changes summary:

  • Introduce a new question that asks whether the user wants to follow an established styleguide, offering SciML Style, Blue Style and YAS Style as options (same of PkgTemplates). By default, no styleguide is selected
  • Now the question about indentation is made conditional and asked only if the user selects "no styleguide" since all other three are opinionated in what to follow (4 white spaces, which is the default choice anyway). An alternative could be to always ask the question about indentation and allow things like "sciml but I use indentation 2".
  • If Blue style or Sciml style is chosen, the corresponding badge is added to the README. As far as I can tell, YAS does not have an official badge.

Related issues

Closes #424

Checklist

@lucaferranti lucaferranti marked this pull request as draft September 5, 2024 21:41
@lucaferranti
Copy link
Contributor Author

@abelsiqueira I marked this as draft as I need to update/fix tests, but feedback on the general direction welcome already.

What do you think about making the question about indentation conditional? I think it's a bit more coherent, since if someone wants to pick a styleguide, might be better for the generator not to encourage them immediately to introduce exceptions.

Another question, I think the linting will fail, because when I ran it locally it was sorting the lines of .JuliaFormater.toml.jinja from

{% if StyleGuide == 'none' %}
indent = {{ Indentation }}
margin = 92
normalize_line_endings = "unix"
{% else %}
style = "{{ StyleGuide }}"
{% endif %}

into the nonsensical

indent = {{ Indentation }}
margin = 92
normalize_line_endings = "unix"
style = "{{ StyleGuide }}"
{% else %}
{% endif %}
{% if StyleGuide == 'none' %}

is there a way to prevent this?

@abelsiqueira
Copy link
Owner

Thanks for the PR!

The Indentation is used in other places - it also serves as the indentation of yaml, json and markdown (I think) files. So it should still be a required question.
One possibility is to separate the Julia indentation from the rest of the code indentation - which I was thinking about doing anyway, because I like yaml 2 spaces even if Julia is 4 spaces. What do you think?

I don't use the styles, so I'm not sure if people follow them fully normally. It might be worth checking some random .JuliaFormatter.toml in the wild. Since I like indent 2, if I were to follow a style, I would probably still try to change it - i.e., add the indent even is style is selected. But we don't have to accommodate everything in the template, only the most expected (or recommended) practice. So probably it is fine to only have style is a style is selected.

PS. I checked a few random files on GitHub searching for path:.JuliaFormatter.toml style and it is a jungle of opinions. So let's keep the style as the only thing there if the user picks a style.

The sorting issue was unexpected, but I found an easy fix. The .pre-commit-config.yaml hook file-contents-sorted is expecting a regex for the files argument, so we should just add a $ at the end of line 19.

@@ -10,6 +10,7 @@
[![Coverage](https://codecov.io/gh/{{ PackageOwner }}/{{ PackageName }}.jl/branch/main/graph/badge.svg)](https://codecov.io/gh/{{ PackageOwner }}/{{ PackageName }}.jl)
[![DOI](https://zenodo.org/badge/DOI/FIXME)](https://doi.org/FIXME)
{% if AddCodeOfConduct %}[![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.md){% endif %}
{% if StyleGuide == 'sciml' %}[![SciML Code Style](https://img.shields.io/static/v1?label=code%20style&message=SciML&color=9558b2&labelColor=389826)](https://github.com/SciML/SciMLStyle){% endif %}{% if StyleGuide == 'blue' %}[![Code Style: Blue](https://img.shields.io/badge/code%20style-blue-4495d1.svg)](https://github.com/JuliaDiff/BlueStyle){% endif %}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can keep them in separate lines. It should be possible to remove the empty spaces if we use {%- and/or -%} in either or both if and endif - yes, 2^4 combinations, because I don't understand how the whitespace removal works yet. Docs are here: https://jinja.palletsprojects.com/en/3.0.x/templates/#whitespace-control
I would try {%- if ... %} ... {%- endif %} first.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked on stack overflow, and the solution for this is to use

{% if stuff -%}
Badge
{% endif -%}

But I will work on this later on #445, so it's fine either way.

copier/essential.yml Outdated Show resolved Hide resolved
@lucaferranti
Copy link
Contributor Author

The Indentation is used in other places - it also serves as the indentation of yaml, json and markdown (I think) files. So it should still be a required question.
One possibility is to separate the Julia indentation from the rest of the code indentation - which I was thinking about doing anyway, because I like yaml 2 spaces even if Julia is 4 spaces. What do you think?

makes sense, I also find 2 spaces indentation more logical for yaml and other files. This decoupling allows to move the question about styleguide to code-quality.yml, which is maybe a more suitable place? I don't think it's an essential question. One could also keep an additional conditional JuliaIndentation question which is asked if the user selects to not adhere to any styleguide.

I'll do the changes and look into the tests later today

@abelsiqueira
Copy link
Owner

Yeah, I think I wanted to select indentation before deciding on strategy, because it will appear in the file regardless of strategy. But it makes more sense to have the style questions in code quality. I think we can move all the indentation questions there.

I also realised that if we split the Indentation, we should also adjust .editorconfig and split the indent definition there.

If you think it is easier you can make a separate PR before this one only to split Indentation and move to code-quality.

@abelsiqueira
Copy link
Owner

Just a heads up, I've added a description field in the copier yml files to further describe what the question is about. When you change the existing questions, you should have some easy-to-fix conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Support styleguides
2 participants