KirillOsenkov
commented
9 years ago Hey @AdamSpeight2008, first off I appreciate your willing to contribute!
However I do see a few things about your pull request that might be preventing it from being accepted. I hope you don't mind.
First thing that jumps to mind is what problem is this change solving. It looks like it primarily changes the style of the codebase. However this codebase is already using a well-established style and I don't see a need to change it to a different one.
When contributing to a project, use their style and formatting (when in Rome...). The style I use in my code is the common C# style used by the Roslyn team and the majority of managed teams at Microsoft. Default VS formatter settings default to that style. If you Ctrl+K,D (or Ctrl+E,D) in your code, it will destroy half of your custom tabular formatting. Being able to auto-format the code on every save is an absolute prerequisite for us.
Additionally, my code is StyleCop-clean (using the ruleset adopted by the Roslyn team). Your code won't pass StyleCop and will be harder to maintain.
When it comes to identifier naming, we on the Roslyn team also avoid abbreviations, and so things like Exts or FMT won't pass any code review. FMT also violates the rule that locals need to start with lowercase. And of course pType or mName represent Hungarian notation, which is an absolute no-no.
Further, we always require that the if statement is always containing a block, and every block has opening curly and closing curly on separate lines. Every if statement if it's not the last one should be followed by a blank line (StyleCop).
Finally, the code is intentionally simple in the meaning of "gets the job done". I chose to avoid introducing additional constructs (such as additional constructors or your Exts class) because the benefit of having them didn't justify additional complexity. I wanted to write this tool and move on, without spending any more time on it.
I put the code out there in hopes that it will help people, however unfortunately I have very limited time to maintain it. I will only fix very critical bugs or maybe accept significant functional improvements (such as if anyone would add VB support, for instance). I will also absolutely enforce formattability, existing coding and styling guidelines, basic StyleCop rules and other commonly used conventions. Sorry about that.
I hope all of that didn't sound too much ivory tower... I don't mean to discourage, however the combination of a) this pull request not adding new and significant value, b) me having very limited time and c) this pull request violating quite a few of coding guidelines that we use I will have to decline it in its present state. If I accept, I will inflict pain upon myself while merging/integrating with an internal fork of it.
Thanks! Kirill Osenkov (@KirillOsenkov)
AdamSpeight2008
Replace the use of List