-
Notifications
You must be signed in to change notification settings - Fork 3
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
gather trailing parens around comments? #60
Comments
This was a doozy of a problem to figure out. Hopefully PR-58 closes this out for good. |
Apologies for the inquiry, but I'm following this project with great interest. Why is the following considered beneficial?
When I read code as in But when I read It appears to me slurping closing parens across comments messes up the intended code-comment pairings. Here's an example that perhaps more closely demonstrates this: (aaa
[bbb
[ccc
ddd ;; need to use ddd here because of JIRA-1234
;; append more items to increase blergh threshold as needed
]
ggg
]) vs (aaa
[bbb
[ccc
ddd] ;; need to use ddd here because of JIRA-1234
;; append more items to increase blergh threshold as needed
ggg]) Especially the second comment looks quite out of place this way. |
Thank you! Your interest is appreciated 🤓
The short answer is compatibility for Parinfer users. See parinfer issue #92 for some historical context. We also have the gather trailing parens recommendation from the Clojure Style Guide, which would apply in the following example: [aaa
[bbb ccc]
[ddd eee]
;; fff
] I will give this problem a think, but I am currently leaning towards the way it is written now. I think the core issue comes down to "should comment lines be considered elements of the form they are in?", and I think the answer should be "no, they are not". There is probably some friction between the process of structural editing (ie: paredit) and that approach. Undoubtedly, how to treat comments will be one of the harder parts of this project. And it is unlikely that the solution will be optimal for all possible code examples. |
Thanks for taking the time to respond!
I can't say I'm a fan of parinfer - it is truly an impressive project which can make it easier for some folks to work with a LISP but I'm on the side of being in full control of the source code for which I find paredit and its derivatives perfect. I do understand your goal of making this tool play nice with all ways of editing code though.
I fully agree. Even paredit-likes don't always do what I expect them to do with comments. On the other hand, paredit doesn't prevent me from updating the code after to place comments where I think they should be.
There's another recommendation about margin comments there that is perhaps even more closely related and it also says "Avoid using those in situations that would result in hanging closing parentheses". I generally agree with this but I think there are exceptions. I'd be interested to know why you think not doing this would break compatibility with Parinfer. I see there is some back-and-forth in the linked issue and even Shaun himself agrees that this is something controversial that Parinfer does and created parinfer/parstager#1 to perhaps mitigate the effect. In my opinion - due to the reasons Shaun laid out - parinfer is more or less forced to make a controversial source code change. Do you think this tool has to necessarily replicate that same controversial edit? My points are:
I agree with this in general, but even if they aren't elements of forms, they are a very important part of the source code itself and often extremely helpful when done well. Just food for thought. Thanks for reading. |
I have given this much hammock time over the last several days and I think the best path forward is to wrap the closing parens "over" the comment. ie: how it is currently implemented via PR-58 both ways are fineIf I were doing a code review and I saw either way of formatting, I don't think I would ever suggest changing to "the other way". In other words, I do not think the difference between these two styles has a large effect on the readability of the program: both are acceptable. Given that, I think it makes sense to pick the way that can always be enforced (wrapping), does not cause friction with Parinfer users, and aligns with the Clojure Style Guide recommendation. it is better to be consistentTo the extent that Standard Clojure Style can always produce the same result, it benefits everyone. I have a strong bias toward "making a decision" instead of "let the user choose". This will not always be possible (ie: this project is not a strict pretty printer), and there are cases where I think it is important to allow the user to nudge the formatter in a direction ("Rule 3" indentation, vertically aligning once it is "one way", I don't think this will be importantColin Fleming has a good quote from the Clojureverse thread that spawned this project:
The comment from user oskarth on this issue is instructive to consider: is the issue here the formatting? Or his expectation of what the formatting should be? I think it is the latter, and if tooling always made it work in the same way then that expectation can go away for everyone. This sort of comment on a Pull Request is exactly the kind of small friction that I hope for Standard Clojure Style to remove for teams who adopt it. Thank you for the interest and thoughtful comments @imrekoszo! If everyone who engages with this project brings the same level of thought and consideration as you, we are sure to all find success 😊 |
Just for clarification – it wasn't entirely clear what "wrapping over" meant, and I had to inspect the commits in the linked PR to figure out what it was implementing. Was the final decision to follow Parinfer's current behaviour? i.e.
I'm surprised that no one has mentioned the
However this does not address the case of non-code comments, which are the main topic of discussion in the ongoing linked Parinfer thread. In standard lisp jargon, the proposed rule could be seen as the "barfing" of the line comment out into the outermost structure containing the first subsequent non-commented line - possibly top-level. (I suggest using this term instead of the potentially ambiguous "wrapping") Since the implied nesting of comments within the code might convey significant information, having a one-size-fits-all formatter violate this structure doesn't seem ideal. Here is a contrived example to illustrate such a risk:
From the reader's perspective, this is of course a semantically invalid transformation. |
Alternative proposalOne surprisingly robust workaround I've found over the years for this class of issues is to use a single comma character [aaa
[bbb ccc]
[ddd eee]
;; fff
,]
(aaa
[bbb
[ccc
ddd ;; need to use ddd here because of JIRA-1234
;; append more items to increase blergh threshold as needed
,]
ggg])
[:attributes
[:size #{:small
:medium
:large}]
[:color #{:red
:green
;; WARNING: do not extend this list!
,}]
[:misc :prompt/user]] (Note: commas are syntactic whitespace in Clojure, so this has zero effect on the expression being read) This happens to play nicely with language-agnostic Parinfer, which treats the comma as a symbol constituent, leaving it and any following delimiters untouched. I don't recall where I first learnt this 'trick', but it seems to be relatively rare / undocumented in the wild. Perhaps this should be brought up on the Clojure style guide repo - if the guide ends up recommending such a usage, ideally the formatter could detect such a situation and automatically insert these anchors:
Note: Since |
@yuhan0 thanks for the alternative suggestion. Sadly, the important point you raise in your previous comment still applies:
i.e. one needs to be aware that the formatter will make or has made a "semantically invalid transformation" and prepare for it/work around it @shaunlebron's suggested resolution of the Parinfer issue appears very well thought out to me. Allowing these still go against the general recommendation about margin comments in the Style Guide, but as @yuhan0 points out, there will be exceptions when you want to be able to add comments like this. |
One non-contrived situation where I suspect this rule will cause annoyance/friction is with "rich comment blocks" and the use of (comment
(range 5)
;; => (0 1 2 3 4)
(interpose '* (range 5))
;; => (0 * 1 * 2 * 3 * 4)
,) This is by far my most common use of the Leaving out any anchor would of course result in the following suboptimal formatting, by current Parinfer rules: (comment
(range 5)
;; => (0 1 2 3 4)
(interpose '* (range 5)))
;; => (0 * 1 * 2 * 3 * 4) |
@yuhan0 just reacting to the examples you added in your earlier comment: I think cases B, C, and D should be left alone by formatters as there might be a semantic importance of the comments being inside the vector. |
Yeah, that was my line of thought originally, but then I find it hard to imagine any scenario where one assigns semantic importance to the nesting of inline comments. Writing such comments 'in the margin' is nearly always an indication that they are directly referring to the code on that line, rather than belonging in any larger block structure. In practice a far more common scenario is the following: an edit is made that leaves dangling parens following an inline comment: eg. deleting the forms (let [x (foo) ;; Warning: side effects!
y (bar x) ;; returns nil if invalid
z (qux y)]
(do-something x y)) Which results in this situation, where users may find difficulty getting their editor to un-dangle the
|
I'd argue that out of those 2, this is the "more correct" version: (let [x (foo) ;; Warning: side effects!
y (bar x) ;; returns nil if invalid
]
(do-something x y)) See also: (comment
(foo 1) ;;=> :foo
(foo 2) ;;=> :bar
) I associate a margin comment with the form preceding it; it isn't the binding vector that I think it's important for formatters to preserve programmer intention. If the tool un-dangles (barfs) any of these, then it isn't respecting the programmer's intention to associate those trailing comments with the forms they were typed directly after. On the other hand, if the tool preserves these dangling closers, then not only intention is preserved, but you can continue to add items to the binding vector/comment form without having to re-dangle (slurp), without having to rely on something like Parinfer. Moreover, allowing dangling closers like this still allows the programmer to place the comment either before or after the closer, so it isn't limiting, whereas forced un-dangling is. The comma/ Bonus: having to add comments like these might be an indication that naming is not the best in the code. Dangling closers are more noticeable in a code review than a "semantically incorrectly placed" closer and might urge programmers better to fix naming. |
As logical as this may seem, I don't believe this is an accurate reflection of real-world practice across various Lisp dialects. For instance, would you consider the final line here here to be "incorrectly" formatted? (defn classify-squares [n]
(->> (range n) ; generate a bunch of numbers
(map #(* % %)) ; calculate their squares
(group-by even?))) ; and sort them by parity I think the vast majority of readers would be unbothered by the fact that its preceding form is at a different nesting level compared to its neighbors. A quick search of Clojure's own source shows a couple of such comments: I don't think either of these are indicative of namings which should be fixed – they're simply a compact way of pointing out something about the code on that line. (Sorry if my toy examples were misleading in that regard) |
Just to clarify my position, I'm not against the existence of 'unanchored' dangling closers per se - it's simply the fact that decades-old norms around formatting Lisp code very strongly proscribe against them. Plus, there is already plenty of existing tooling out there which enforce these conventions. I believe the goal of a Standard Style formatter project like this one should be to produce formatted code that is as minimally controversial as possible in order to encourage adoption, while being just opinionated enough to avoid bikeshedding over minor details that it leaves untouched. (whether or not this tradeoff is possible remains to be seen.. indentation seems to be a dealbreaker for many) Note that this is in contrast with tools like Parinfer, which is concerned with the ergonomics of real-time editing, and has to focus on being minimally invasive and respect one's intent on a per-keystroke basis. As seen in the other thread, this puts pressure on Parinfer to compromise on matters such as allowing the presence of those dangling closers. On the other hand, a formatter is something you'd run perhaps at the end of an editing session, or on every file-save, so the tradeoffs to be made should be considered differently - eg. that it could help in 'cleaning up' those states left by Parinfer. I've tried to express this with the (I realise that the bit about automatic comma-placement might have been the most controversial.. My reasoning was somewhat of a 'defensive' one – to reduce conflict with other tools – something I'm less eager to justify now compared to the other points.) |
@yuhan0 I do find your
I think preserving intent is important no matter when the tool runs. It might even be more important for a check-in-time tool as you might not notice the tiny-yet-intent-hiding edit it makes when all you're thinking about is to finish the coding session.
Wait till you learn that this tool throws out the decades-old Lisp tradition of semantic indentation in favour of fixed indentation 😅 Edit: fix typo |
Yep, I'm aware of the indentation matter and I like the direction the tool is going there. My point was that such a deviation from tradition shouldn't be taken lightly – in that case the tradeoffs of REPL-driven vs static analysis were judged to be worth the cost of breaking tradition, a consideration that doesn't apply here. In any case I'm not claiming to be the best judge of user intent / formatting choices etc, but I'm curious @oakmac what procedure you follow in such situations where there might not be a clear consensus on the Right Thing to do? (i.e. not covered by the existing Clojure Style guide either) |
@imrekoszo This isn’t true, right? It compromises the two approaches by letting the user set the “semantic” indentation of the first line, then it “fixes” the rest of the lines to follow it. |
Just a quick note to say that I am monitoring the discussion in this thread and think there are some great points being raised. I am confident that the current behavior (as of v0.3.0) will need to change. |
I agree with "Desired format" here. If I saw the "After edit" format on a code review, I would probably suggest changing it to "Desired format", and that suggestion would be a nit, which is exactly the sort of PR comment that I hope Standard Clojure Style helps people avoid having to make. |
I agree with this and think Standard Clojure Style needs a good answer / solution here. I really like the I do not want Standard Clojure Style to add the comma automatically, but it may be necessary to promote "if you really do not want Standard Clojure Style to gather your trailing parens to one line, use a comma to hold them" advice. |
I should clarify that Standard Clojure Style is not being designed as a "tool for Parinfer users" anymore than it is a tool for Paredit users or users who use no editor tooling to help manage their parens (yes - there are people who work in this way). It needs to support as many Clojure developers as it can, work in as many environments as it can, and reduce friction and diffs between users who edit their code in different ways. In this particular use case, there is friction between Parinfer users and Paredit users because Parinfer users never have the condition of trailing parens below a single comment line, whereas a Paredit user might actively choose the code to look that way. It is not "one group thinks it should look one way, and the other way is wrong". It is "one group doesn't think about this at all, and the other group sometimes does". I think that the idea of a |
The rules for margin comments in the Clojure Style Guide seemed off to me, so I’m proposing a clarification here: bbatsov/clojure-style-guide#258 |
Ha, I like this trick and completely forgot about it. One thing to consider is that if the formatter requires the comma to be there, but doesn’t add the commas automatically, then it will create a lot of initial diffs in existing codebases that have to be manually corrected per project. Assuming I understand this correctly, if we want to avoid those diffs, this means either 1) Standard Clojure Style has to add them in automatically, or 2) Parinfer has to be updated to not require them. |
@shaunlebron by fixed indentation I meant "Multi-line lists that start with a symbol are always indented with two spaces" per the Nikita's article, with the addition of your rule "3. multi-line lists are indented to "first-arg alignment" if and only if the 2nd line is indented as such". (It would perhaps be useful to give a name to this composite style, perhaps Soft Fixed or Hintable Fixed indentation 😅 ) By semantic indentation I meant what's in Bozhidar's article, with further details in the CIDER Indentation, and Indentation Specification articles. The bottom line of these is that formatting needs to understand the semantics of forms and apply different formatting rules based on those semantics (special args indented one space, body indented two spaces, etc.) In the fixed vs semantic debate I agree with the Hintable Fixed position due to its general applicability (formatter doesn't need to know the semantics), but still letting the programmer (who does know) to align with the first arg if they consider that more expressive. On the other hand, this allowing of Rule 3 carries the potential of PR "nit" comments, similar to what @oakmac points out above. Both the following are valid according to Standard Clojure Style, but only one each per either Semantic of Strict Fixed. ;; semantic ❌ strict fixed ✅ hintable fixed ✅
(map +
(range 10)
(range 10))
;; semantic ✅ strict fixed ❌ hintable fixed ✅
(map +
(range 10)
(range 10)) For the sake of completeness, here are the narrow formats too, of which each of the 3 formatting specifications only allow one: ;; semantic ✅ strict fixed ❌ hintable fixed ❌
(map
+
(range 10)
(range 10))
;; semantic ❌ strict fixed ✅ hintable fixed ✅
(map
+
(range 10)
(range 10)) I suppose there is a symmetry between "requiring a comma to prevent barfing the comment" and "requiring (manual/tool-assisted) lining up the first arg in the second line with the first arg, to prevent the strict 2-space indent". Both of these require effort from the programmer in case they expect a benefit from the "alternative" formatting. Commas for this purpose are quite nonintrusive actually, now that I think about it. A little more work than just pressing enter, but not too bad. Similar effort to lining up that nth argument. While aligning with the first arg is common practice, leaving commas around is less so, and Shaun is right to point out the possibility of additional initial diffs. Note that since the narrow indentation format is changing from semantic to fixed, that will probably bring even more initial diffs. However, these indentation changes in my opinion preserve programmer intent better than barfing a trailing margin comment, but again, it could be just me. |
It is not a goal of Standard Clojure Style to "avoid initial diffs when applied to a codebase for the first time". I suspect that for 99% of forms, gathering the trailing parens to one line generally improves the readability of the code. The "comment line holding trailing parens" is the exception, not the norm. |
Agree - the more I think about this the more I like it as a solution. "Standard Clojure Style will always gather trailing parens to a single line (as recommended by the Clojure Style Guide and enforced by Parinfer). If you want a |
@oakmac I think it's still beneficial to allow indented comments to preserve their position in the AST, per parinfer/parinfer.js#92 (comment) |
I was thinking about changing the Parinfer enforcement, but maybe that ship has sailed. I’m at least leaning toward an option for auto-inserting the commas. I requested another clarification in the Clojure Style Guide to see if we can add a recommendation there about indented comments, or at least a mention. |
PR-89 supports the ability for commas to "hold" trailing parens on the next line. |
Relevant to this thread, I learned a fun way to "hold" parens on the next line from @aiba at Clojure/conj 2024. Example rich comment block: (comment
(something)
(something-else :foo :bar)
(another-thing)
:-) |
WIP with PR-58
The text was updated successfully, but these errors were encountered: