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

EditorConfig file added and documented #203

Open
wants to merge 7 commits into
base: pages-source
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion content/contributing/cs-coding-style.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ The general rule we follow is "use Visual Studio defaults". For details check th

1. We use [Allman style](https://en.wikipedia.org/wiki/Indent_style#Allman_style) braces, where each brace begins on a new line. Even a single line statement block should go with braces and nested in other statement blocks that use braces.
2. We use four spaces of indentation (no tabs).
3. We use `_camelCase` for internal and private fields and use `readonly` where possible. Prefix static fields can be used with `s_` and thread static fields with `t_`. When used on static fields, `readonly` should come after `static` (i.e. `static readonly` not `readonly static`).
3. We use `_camelCase` for internal and private fields and use `readonly` where possible. Prefix static fields with `s_` and thread static fields with `t_`. When used on static fields, `readonly` should come after `static` (i.e. `static readonly` not `readonly static`).
Copy link
Member

Choose a reason for hiding this comment

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

we don't reinforce the s_ and t_. IT's just a recommendation. But we do reinforce the _

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In dotnet's .editorconfig there is a rule that checks for this. You'll get a message that says "naming rule violation". As a simple contributor I would read that as a "must".

Also: see nanoframework/nanoFramework.Networking.Sntp#134 about this. I'm confused what is expected of me. I think if you produce a message about it, then it is part of the coding style.

Leniency is another question: how much can you deviate from the coding style? That is: when will your contribution be rejected because you first need to clean up the code. I guess if only the "s_" are missing, the contribution is accepted. But there may also be other violations that are acceptable...

Copy link
Member

Choose a reason for hiding this comment

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

For automated checks we have to choose one or the other. I'm afraid we can't have recommendations...
If we are stating that we are following .NET code style (which we aim to) then let's play ball and have those s_ prefixes.
No harm with that, it's just a convention.

Now, we will have extra work to "fix" these as we add the rules to the class repos and update the native code. But that's life. 😅

Copy link
Member

Choose a reason for hiding this comment

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

As for the contribution being accepted or not, considering that we'll have the editorconfig and will enable the check during the build in the pipeline, I suppose that makes it quite clear.

There is always a human reviewing the PRs and we try to be very friendly with the review comments. So not much to worry about.

For existing code, there will an extra step as we add the rules to the repos which will be making sure the code is compliant and make the necessary fixes.

4. We avoid `this.` unless absolutely necessary.
5. We always specify the visibility, even if it's the default (i.e.
`private string _foo` not `string _foo`). Visibility should be the first modifier (i.e.
Expand Down
Loading