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

Make Sass formatting consistent #1013

Merged
merged 10 commits into from
Feb 19, 2024
Merged

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Feb 18, 2024

Part of #975.

This is a PR that formats every single scss file to use single quotes and an indentation of 2 spaces. It was done using VSCode's default formatter + find-and-replace. I chose single quotes and 2 spaces because they were the most frequently used style, but I'd be happy to discuss alternatives.

This will probably end up introducing a lot of merge conflicts with old PRs, due to how large the changes are.

If a consensus is reached on how Sass files should be formatted, perhaps we can automate checking them using Github Actions in a future PR.

@BD103
Copy link
Member Author

BD103 commented Feb 18, 2024

Tip: check "Hide whitespace" when viewing the diff on Github to filter out all indentation-related changes.

Copy link
Member

@TrialDragon TrialDragon left a comment

Choose a reason for hiding this comment

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

Hmm. I like that this is a step towards cleaning stuff up, but I'm unsure about the use of single quotes; my personal preference being double quotes. Curious about others' thoughts and a consensus.

@alice-i-cecile
Copy link
Member

I like double quotes too. Definitely a bikeshed, but we should pick one and standardize.

@BD103
Copy link
Member Author

BD103 commented Feb 18, 2024

I actually prefer double quotes too, I just chose single to be consistent.

Does anyone have complaints about 2-space indentation? I personally prefer 4, but in this case I don't feel strongly in either direction.

@TrialDragon
Copy link
Member

TrialDragon commented Feb 18, 2024

I prefer 2-space indentation for web stuff (fine with 4-space in stuff like Rust) and it's what the default formatter for a lot of the web dev stuff prefers I think. (To the point I feel like I'm constantly having to fight with tooling to keep the 4-space currently in place.)

Mainly because html can get far off the page with 4-space, and maybe ditto for complex Sass since it relies on more indentation.

@doup
Copy link
Contributor

doup commented Feb 18, 2024

I personally would skip any discussion about single/double quotes (or similar code formatting stuff) unless we're willing to add tooling (probably not worth it). Without tooling it will eventually just loose consistency again. :-)

But, indentation consistency is something we can (probably[0]) achieve easily by just adding an .editorconfig file. Btw, it's not only SASS files that are inconsistent, the templates are also inconsistent.

In this PR, I would:

  • add an .editorconfig file with indentation rules for SASS/HTML (or any other relevant file type)
  • reformat both SASS and HTML files

0: I'm quite sure that .editorconfig is acknowledged by most editors… but I might be wrong

@TrialDragon
Copy link
Member

TrialDragon commented Feb 18, 2024

Helix (the primary editor I use) doesn't yet; has open PR for a year now: helix-editor/helix#1777
VSCode / VSCodium (other editor I use) needs a plugin for it.

@alice-i-cecile
Copy link
Member

Yeah, I'd be down for a simple linter for these files, but don't feel strongly. I'll leave that decision to the folks with web dev skills.

@doup
Copy link
Contributor

doup commented Feb 18, 2024

Ok, nevermind 😅

If .editorconfig is not widely supported… then, maybe, another approach without adding extra tooling for formatting (node+prettier?) is to configure the pipelines "super-linter" to enforce some indentation? (if it's possible)

Btw, I was checking what's the most used indentation, and noticed that a lot of changes (and inconsistencies) were introduced in the new book PR (from 4 spaces, to 2 spaces). I personally wouldn't allow arbitrary indentation changes in PRs (looks like some IDE configuration messed few files?). With a linter, at least these huge indentation changes wouldn't happen.

For the record, I don't mind 2/4 spaces, single/double quotes… what I just mean is that formatting should be ideally automated, or if that's not possible then linted/enforced. Every other option is fragile.

@doup
Copy link
Contributor

doup commented Feb 18, 2024

Looks like "super-liner" can be configured to enforce .editorconfig via editorconfig-checker… maybe that could work.

@TrialDragon
Copy link
Member

Btw, I was checking what's the most used indentation, and noticed that a lot of changes (and inconsistencies) were introduced in the new book PR (from 4 spaces, to 2 spaces). I personally wouldn't allow arbitrary indentation changes in PRs (looks like some IDE configuration messed few files?). With a linter, at least these huge indentation changes wouldn't happen.

That's the aforementioned fighting to keep the 4-space indentation. Whenever I write / save a file it will auto format it unless I remember to turn that off every time I open a file. Sorry.

@TrialDragon
Copy link
Member

Looks like "super-liner" can be configured to enforce .editorconfig via editorconfig-checker… maybe that could work.

That can work; at least as a start.

.github/workflows/ci.yml Show resolved Hide resolved
.editorconfig Show resolved Hide resolved
@BD103
Copy link
Member Author

BD103 commented Feb 18, 2024

I switched the Sass files to use double quotes. Additionally, I added a .editorconfig file to enforce this style using Super Linter in Github Actions.

Copy link
Member

@TrialDragon TrialDragon left a comment

Choose a reason for hiding this comment

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

LGTM. I'd be interested in a follow-up PR that does similar things to HTML files since they also suffer from similar indentation issues.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review Ready for a maintainer to consider for merging C-Code Quality labels Feb 18, 2024
@TrialDragon
Copy link
Member

Auto merge may not work; the CI is acting weird with markdownlint showing in spite of being renamed.

@TrialDragon
Copy link
Member

Do we need to reverse the name change of the CI job or something in order for CI to work again? This is weird.

@BD103
Copy link
Member Author

BD103 commented Feb 19, 2024

Do we need to reverse the name change of the CI job or something in order for CI to work again? This is weird.

A setting needs to be changed in the repository settings to update which jobs are required. @alice-i-cecile, could you manually merge this?

@alice-i-cecile
Copy link
Member

I've swapped the markdown-lint job for super-linter in our branch protection checks now :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 19, 2024
Merged via the queue into bevyengine:main with commit bbc525d Feb 19, 2024
9 checks passed
@BD103 BD103 deleted the sass-pass branch February 19, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants