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

Add CSharpier formatting #100

Merged
merged 4 commits into from
Mar 24, 2023
Merged

Add CSharpier formatting #100

merged 4 commits into from
Mar 24, 2023

Conversation

irahopkinson
Copy link
Contributor

@irahopkinson irahopkinson commented Mar 23, 2023

  • add VSCode extension for CSharpier

  • add GHA check

  • specify editor width

  • format existing C# files

  • format TS and other existing files

It may be easier to review the 3 commits separately. The first makes the changes, the second has C# re-formatting (shorter width), the third TS has other file re-formatting (longer width).


This change is Reviewable

@irahopkinson irahopkinson enabled auto-merge (squash) March 23, 2023 21:19
Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r1, 7 of 7 files at r2, 42 of 42 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @irahopkinson)


README.md line 116 at r1 (raw file):

## Formatting and Linting

Formatting happens automatically when you commit. If you use VS Code with this repo's recommended extensions it will format when you save.

How does formatting automatically happen when you commit? I expected to see changes to the husky file. That's where the pre-commit linting happens.


.vscode/settings.json line 6 at r1 (raw file):

    "editor.defaultFormatter": "csharpier.csharpier-vscode"
  },
  "editor.formatOnSave": true,

I would prefer not to mandate format on save to be honest. I personally like not having to format until I want to. We can certainly discuss sometime if desired. I also don't see anywhere where prettier is configured to use width 100 - where is that? I expected to see changes in the prettier config file. Are we relying on VSCode's config to tell prettier how to run, or can prettier format to 100 independently?


c-sharp/Program.cs line 10 at r2 (raw file):

public static class Program
{
    public static async Task Main( /* string[] args */

Eeewww... Do you know why this formatting is being applied? Can we change it?


c-sharp/Web/PapiClient.cs line 135 at r2 (raw file):

                Console.WriteLine("Exception while handling response message: {0}", ex);
            }
        } while (response == null);

I personally prefer to have the while after the } in C# because the do is on the line above. Just feels more parallel. Same with try and catch. Is there a rule for this? What do you think?

Copy link
Contributor Author

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

Reviewable status: 57 of 58 files reviewed, 4 unresolved discussions (waiting on @tjcouch-sil)


README.md line 116 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

How does formatting automatically happen when you commit? I expected to see changes to the husky file. That's where the pre-commit linting happens.

It's already working - this is just documenting that it does do it on commit in the README. Husky precommit calls lint-staged which has config on L68 of package.json. Additionally the new c-sharp/.lintstagedrc.yml file now ensures it happens for C# also. lint-staged combines the package.json config using prettier with this file for C#. I specifically kept them separate so they can be handled in the expected locations for each.


.vscode/settings.json line 6 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I would prefer not to mandate format on save to be honest. I personally like not having to format until I want to. We can certainly discuss sometime if desired. I also don't see anywhere where prettier is configured to use width 100 - where is that? I expected to see changes in the prettier config file. Are we relying on VSCode's config to tell prettier how to run, or can prettier format to 100 independently?

Yeah, I debated about adding this one. I included it because it gets formatting happening in the editor sooner. Its pretty jarring to leave until commit but thats an importnat backup. I had assumed if it was on in the Workspace Settings you could still turn it off in the User Settings but I just tried it and that doesn't work. Do you know you can Save without Formatting in VS Code (which has a long keyboard shortcut but you can change that)? Would that help you?

Prettier reads compatible config from .editorconfig (see max_line_length = 100 there), that way you only have to specifiy it in one place and its used in both (particularly useful if using some other editor that only respects .editorconfig).


c-sharp/Program.cs line 10 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Eeewww... Do you know why this formatting is being applied? Can we change it?

Yeah that was my reaction to that change also - I tried several things to change this to no avail. CSharpier is opinionated for a reason. However after looking at the way it formated other files with args (e.g. see PapiClient.cs below), it does make more sense and this is just an unfortunate edge case. One option is to remove the comment. I've 'fixed' it for now by putting the comment on its own line.


c-sharp/Web/PapiClient.cs line 135 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I personally prefer to have the while after the } in C# because the do is on the line above. Just feels more parallel. Same with try and catch. Is there a rule for this? What do you think?

Sorry, no rules, CSharpier is opinionated for a reason - to stop endless formatting debate. The only thing we can change in CSharpier is already specified in the c-sharp/.csharpierrc.yml file.

Copy link
Member

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

I scanned through all the updates. The C# ones all make sense to me with the rules. There are a couple of small things, but as you mentioned in other comments it's opinionated for a reason. Nothing here seems egregious, and in all it seems better to have enforced consistency than not, especially when talking about working with a larger group of people spread across multiple organizations.

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

:lgtm: Agreed. Although some of the formatting is a bit unfortunate, it is definitely better to have formatting than not to have it :) thanks for all the hard work on this!!

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)


README.md line 116 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

It's already working - this is just documenting that it does do it on commit in the README. Husky precommit calls lint-staged which has config on L68 of package.json. Additionally the new c-sharp/.lintstagedrc.yml file now ensures it happens for C# also. lint-staged combines the package.json config using prettier with this file for C#. I specifically kept them separate so they can be handled in the expected locations for each.

Aaaah that's neat that it runs the .lintstagedrc.yml in directories. Thanks for explaining!


.vscode/settings.json line 6 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Yeah, I debated about adding this one. I included it because it gets formatting happening in the editor sooner. Its pretty jarring to leave until commit but thats an importnat backup. I had assumed if it was on in the Workspace Settings you could still turn it off in the User Settings but I just tried it and that doesn't work. Do you know you can Save without Formatting in VS Code (which has a long keyboard shortcut but you can change that)? Would that help you?

Prettier reads compatible config from .editorconfig (see max_line_length = 100 there), that way you only have to specifiy it in one place and its used in both (particularly useful if using some other editor that only respects .editorconfig).

You have a good point - I'll just give it a shot and try to get used to it. I will try the Save without Formatting thing if I can't get used to the spots that I imagine would be annoying :)

Ah cool! Thanks for sharing. Good to know.

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @irahopkinson)

@irahopkinson irahopkinson merged commit ee4ee2b into main Mar 24, 2023
@irahopkinson irahopkinson deleted the 77-cshapier branch March 24, 2023 17:45
@irahopkinson irahopkinson linked an issue Mar 24, 2023 that may be closed by this pull request
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.

Evaluate ERB eslint rules
3 participants