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

Change the default indentation from 2 to 4 #407

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Change the default indentation from 2 to 4 #407

merged 1 commit into from
Aug 22, 2024

Conversation

abelsiqueira
Copy link
Collaborator

@abelsiqueira abelsiqueira commented Aug 13, 2024

This is the default in the Julia ecosystem, both in the official guidelines and on various styles.

The Julia files in the template have been changed to use 4 spaces.
The other files in the template are still using 2 spaces.
This should be changed to 4 when pre-commit is run for the first time.

Update the consistency tests that verifies Copier's API to run the
pre-commit before the comparison, to ensure that formatting changes
don't influence the tests.

Related issues

Closes #403

Checklist

@abelsiqueira abelsiqueira force-pushed the 403-indent branch 2 times, most recently from 763d244 to 4fb7b34 Compare August 13, 2024 13:11
This is the default in the Julia ecosystem, both in the official
guidelines and on various styles.

The Julia files in the template have been changed to use 4 spaces. The
other files in the template are still using 2 spaces. This should be
changed to 4 when pre-commit is run for the first time.

Update the consistency tests that verifies Copier's API to run the
pre-commit before the comparison, to ensure that formatting changes
don't influence the tests.

Closes #403
Copy link

codecov bot commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (90b3862) to head (b417f6e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #407   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines           81        81           
=========================================
  Hits            81        81           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@abelsiqueira abelsiqueira marked this pull request as ready for review August 13, 2024 13:30
@abelsiqueira abelsiqueira requested a review from fdiblen August 13, 2024 13:30
@abelsiqueira
Copy link
Collaborator Author

This took longer than expected. The changes was simple, but the tests failed because the default formatting changed between commits. A simpler PR could be not change the template files' format, but the issue would eventually bite, so I've updated the tests to run pre-commit before comparing the resulting packages.

One thing I am not too pleased about this is that now the normal tests also require installing pre-commit.

@fdiblen, I've request your review. If you want to discuss at the office, I'll be there Thursday.

@oxinabox, let me know if you want to review as well.

Copy link
Contributor

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

I wonder if the correct play to do this more robustly,
is to run JuliaFormatter on the generated files, with the given indent setting, (and possibly no others)
after running copier.
So it doesn't actually matter what was in the files being generated.

Anyway, this looks good to me.

@abelsiqueira
Copy link
Collaborator Author

I wonder if the correct play to do this more robustly, is to run JuliaFormatter on the generated files

The pre-commit includes JuliaFormatter, so it is doing that now in the tests.
Thanks for the review!

Copy link
Collaborator

@fdiblen fdiblen left a comment

Choose a reason for hiding this comment

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

I went over all the changes and it looks good! Thank you, Abel!

@abelsiqueira abelsiqueira merged commit 50a3d1a into main Aug 22, 2024
8 checks passed
@abelsiqueira abelsiqueira deleted the 403-indent branch August 22, 2024 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Default indentation level should be 4 spaces
3 participants