-
Notifications
You must be signed in to change notification settings - Fork 844
gremlint and gremlin-mcp Improvements #3289
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
base: 3.8-dev
Are you sure you want to change the base?
Conversation
If the initial argument to a step exceeded max line length, gremlint would immediately line break and indent. It never looked like well formatted Gremlin in those cases. Changed it so that the initial argument would stay on same line and then align remaining parts to that column where the argument started. Added greedy argument packing
| .count(local) | ||
| .is(gt(1)))`); | ||
| .filter(select(values) | ||
| .count(local) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this needs to be changed but to me this looks very odd. In Java code, when you break a chain of methods for something like fluent APIs into separate lines, there would usually be extra indentation to show which part the next line is supposed to be connected to. Having the starting . line up exactly with the step before it just looks a bit odd. Again, just pointing this out in case anyone else feels the same but I probably wouldn't change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't like the whole dots starting a line at all in any event - almost wanted to remove the feature and get rid of the complexity. i guess there could be special rules in this case to further indent, but i'm not sure what this should be for Gremlin exactly.. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be good to have another test to show what should be done in this case rather than it just not crashing. This change can push the indentation of the new line further right, which means Traversals with more nesting will now have a higher chance of hitting the maxLineLength.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm...not sure what to actually write for a test. not really even sure what maxLineLength even means when it's smaller than the longest individual token. i guess that could all be explored, but i'm not sure i want to encode anything with tests right now for that.
there's a lot of open issues on gremlint - i think this is just another one: https://issues.apache.org/jira/browse/TINKERPOP-3224
|
VOTE +1 |
Improved gremlint with better argument formatting and added Gremlin formatting via gremlint as a feature to gremlin-mcp.
VOTE +1