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

Break and Tab Parameters with Attributes #333

Merged
merged 3 commits into from
Jun 23, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
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: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
# Jetbrains doesn't say to ignore these, but it seems like they shouldn't be committed
**/.idea/**/watcherTasks.xml

.vscode
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


node_modules
bin
obj
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,10 @@ class ClassName

public void MethodName(
int someParameter,
[LongAttributeName(SomeFlag.SomeValue)]
AnotherObject anotherObject
[ShortAttributeName] AnotherObject anotherObject,
[VeryLongAttributeName(SomeFlag.SomeValue, SomeOtherFlag.SomeOtherLongValue)]
string tabbedBreakParameter,
bool anotherParameter
) {
return;
}
Expand Down
25 changes: 19 additions & 6 deletions Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/Parameter.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
using System;
using System.Collections.Generic;
using CSharpier.DocTypes;
using CSharpier.SyntaxPrinter;
using CSharpier.Utilities;
using Microsoft.CodeAnalysis.CSharp.Syntax;

Expand All @@ -10,24 +10,37 @@ public static class Parameter
{
public static Doc Print(ParameterSyntax node)
{
var hasAttribute = node.AttributeLists.Any();
var groupId = hasAttribute ? Guid.NewGuid().ToString() : string.Empty;
var docs = new List<Doc>
{
AttributeLists.Print(node, node.AttributeLists),
node.AttributeLists.Any() ? Doc.Line : Doc.Null,
hasAttribute ? Doc.IndentIfBreak(Doc.Line, groupId) : Doc.Null,
Modifiers.Print(node.Modifiers)
};

var paramDocs = new List<Doc>();
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 (node.Type != null)
{
docs.Add(Node.Print(node.Type), " ");
paramDocs.Add(Node.Print(node.Type), " ");
}

docs.Add(Token.Print(node.Identifier));
paramDocs.Add(Token.Print(node.Identifier));
if (node.Default != null)
{
docs.Add(EqualsValueClause.Print(node.Default));
paramDocs.Add(EqualsValueClause.Print(node.Default));
}

return Doc.Concat(docs);
if (hasAttribute)
{
docs.Add(Doc.Concat(paramDocs));
return Doc.GroupWithId(groupId, docs.ToArray());
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@belav belav Jun 22, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could solve that issue as part of this, not a big issue to do so.

}
else
{
docs.AddRange(paramDocs);
return Doc.Concat(docs);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using CSharpier.DocTypes;
using CSharpier.SyntaxPrinter;
using Microsoft.CodeAnalysis.CSharp.Syntax;

namespace CSharpier.SyntaxPrinter.SyntaxNodePrinters
Expand Down