Closed ClayChipps closed 3 years ago
G'day @ctchipps Thanks for the submission. As you say, this is "religious war" territory. 😂
Let @daveespo, @stohn777, and I take some time and see what we want to include and we will get you feedback.
Cheers!
Hi @ctchipps ,
Thanks for this PR -- we discussed it at our monthly review meeting and here's the summary, in no particular order :-)
.editorconfig
and .prettierrc
are long overdue for this project. We agree with the spirit of this PR and want to get them committed so our undocumented 'style guide' is documented in code. fflib-apex-mocks
uses tabs for indentation. There are 155 downstream forks of this repo and thousands of others have just taken this code as is and incorporated it directly into their projects. By reformatting every file, it makes for unnecessary merge conflicts for those that have made local changes to these classes for good reason. For this reason, we have decided to standardize on tabs over spaces for indentation in Apex classessfdx-project.json
and project-scratch-def.json
) are really only in this project to permit it to be deployed and run standalone -- it's unlikely that anyone has customized those two files in their downstream fork so we should stick with the standard practice of two space indentation for JSON files -- this is the way that the aforementioned files are indented when creating a new project using force:project:create
so I don't think we need an editorconfig override for sfdx-project.json
README
in a new "Contributing" section -- please add the instructions as part of the README
** Did you use the https://github.com/dangmai/prettier-plugin-apex plugin? I tried using this to replicate your formatting results and had an error returned for every Apex class file:
[error] sfdx-source\apex-mocks\test\classes\mocks\fflib_Mocks.cls: Error
[error] at parseTextWithSpawn (.....\fflib\prettier-fflib-apex-mocks\node_modules\prettier-plugin-apex\src\parser.js:36:11)
You may be asking: what about fflib-apex-common
and force-di
and at4dx
?
fflib_SObjectDomain
and fflib_SObjectSelector
. Most of the other classes are predominantly tab-indented and fairly consistent with regard to curly braces (on the end of the line). For this reason, the same reasoning as with fflib-apex-mocks
appliesfflib-apex-mocks
so the same logic applies w/r/t minimizing merge conflictsI'll admit that I've never used Prettier in an Apex project so if there are any traps that the above decisions walk us into, please chime up before revising this PR :-)
Yes, I did use Dang Mai's apex plugin for the formatting. Did you install the latest version of prettier, and if so, are you on JDK 11? Typical errors from the prettier apex plugin are from path length being too long (if you are on windows), or recently prettier 1.9.0 requires JDK 11, as opposed to JDK 8. I was able to recreate the formatting on a different machine today.
In my experience I don't see any of the above decisions leading us into any traps. We've reliably used the apex prettier plugin on a number of projects and thus far the AST has never lead us astray. Dang Mai has done a really good job ensuring that the AST of the original code and formatted code is always the same, and I've never ran into a scenario where the post-formatted code behaves any differently then pre-formatted code.
In an attempt to end the religious code formatting wars, there had been some prior discussion about using tools such as Prettier coupled with EditorConfig in order to enforce a baseline standard of formatting for new additions to the codebase, and to minimize the erroneous whitespace changes that many PRs across all of the AEP repos introduce.
In that spirit, this PR introduces the following changes:
This should provide a solid foundation for the standardization of formatting for the repo, given the following considerations.
Given the above, I think that the EditorConfig configuration is up for debate and consideration. I used a configuration that has worked well for our teams in the past, but as always, we should decide on what configuration makes the most sense for the AEP projects. I am open and happy to hear some feedback regarding this, and happy to make a couple of iterations to see what pans out.
The repo has been in a relatively stable place for a while now, and I think this is a good time for us to figure this out for the mocks repo, as it is one of the most stable AEP projects.
This change is