Break and Tab Parameters with Attributes #333
Conversation
| @@ -26,6 +26,8 @@ | |||
| # Jetbrains doesn't say to ignore these, but it seems like they shouldn't be committed | |||
| **/.idea/**/watcherTasks.xml | |||
|
|
|||
| .vscode | |||
There was a problem hiding this comment.
I don't know if it makes sense to ignore the whole directory.
There are a couple of things that are useful to share between people in the team (e.g. build tasks, extensions, format-on-save etc.)
Why don't you just commit your project specific settings?
There was a problem hiding this comment.
I typically ignore the root .vscode folder until someone has those reusable config items mentioned. In my case I'm using VsCodium which needs to leverage an environment path-specific debugger. Folks don't need my dev environment items in their codebase.
If a useful item pops up from another dev, they could add a caveat line to allow it.
| Modifiers.Print(node.Modifiers) | ||
| }; | ||
|
|
||
| var paramDocs = new List<Doc>(); |
There was a problem hiding this comment.
If I'm thinking this through correctly, I think paramDocs can go away.
The only Doc type that is added to it is a Concat, and nested Concats can just be flattened into a single Concat without affecting the result.
There was a problem hiding this comment.
At a certain point it was holding onto a separate grouping logic, but that's no longer the case so yes it can be simplified now!
| if (hasAttribute) | ||
| { | ||
| docs.Add(Doc.Concat(paramDocs)); | ||
| return Doc.GroupWithId(groupId, docs.ToArray()); |
There was a problem hiding this comment.
I'd suggest adding a new GroupWithId overload that works with List<Doc>, I'm guessing I never ran into needing it which is why it doesn't exist yet.
I did some performance testing a few weeks back and doing unnecessary ToList and ToArray can add up. My testing was inside of the actual Concat methods, which would be hit way more often than this will, but it may still make a noticeable difference.
CSharpier.Benchmarks/Program.cs can be run in release mode if you ever want to play around with benchmarking what some changes may do to the performance.
There was a problem hiding this comment.
Just realized @shocklateboy92 had a similar idea and created an issue for it #334
So you can just leave this as is for now and it can be updated when someone grabs that issue
There was a problem hiding this comment.
I could solve that issue as part of this, not a big issue to do so.
Added breaking and tab logic to parameter when attribute is involved, while grouping the parameter itself as that allowed for a custom break. Removed unnecessary
usingstatement.Ignored
.vscodefor people who think that it is an IDE.Fixes #204.
Fixes #334.