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

handling of comments when sorting content of libraries #12

Open
raphael-proust opened this issue Mar 12, 2025 · 4 comments
Open

handling of comments when sorting content of libraries #12

raphael-proust opened this issue Mar 12, 2025 · 4 comments
Assignees

Comments

@raphael-proust
Copy link

Given an input with a libraires field looking like

 (libraries
  aa
  dd
  ;; this a comment
  zz
  bb
  cc)

I get an output with the libraries sorted, and the comment at the same original index:

 (libraries
  aa
  bb
  ;; this a comment
  cc
  dd
  zz)

I don't know if there is a sane thing to do… in my use-case the comment is a sort of header indicating that the dependencies listed below it have a certain property (think ;; below are expect-test dependencies or ;; below are external dependencies).

I don't know that dunolint can have sane default for those. I guess it could have options for "treat comments as section separators" (ordering in between each comment but not globally) and "treat comments as attached to line below it" (making the comment follow the lib name), but I don't know if that's a sane thing to do.

Alternatively, if it sees comments on their own lines, it could refuse to sort and emit a warning.

@mbarbin
Copy link
Owner

mbarbin commented Mar 12, 2025

Hello @raphael-proust , thanks a lot for opening this issue.

I acknowledge this is an issue. I did notice this while trying the linter in a few random projects, but so far haven't really known what to do about it. I appreciate that you hint that this is not necessarily something with a trivial solution, and I am thankful for your suggestions.

To give you some more context, on the top of my head, after I published sexps-rewriter (the rewriter that dunolint uses under the hood) I did pause and wondered whether I should rather have made it work on top of the CST parser (the one that parses the comments too). However, I found some peace in that, I find the CST ASTs less convenient to traverse and use. But, yes, this is unfortunate. I have sort of a plan, for this specific field, to however reparse the libraries section of the dune files, with a CST like parser, so we'd be able to have more control over what we want to do, including something like your suggestions.

Meanwhile, I did include a common case, that of a comment directly following the lib name on the same line, such as:

 (libraries
  aa
  dd ;; this a comment
  zz
  bb
  cc)

In which case, currently dunolint keeps the comments attached to the lib (dd in this case) when reordering. That's a start I guess!

Another thing worth mentioning is that I have somewhere drafted something like directives, which are ignored by dune (comments with a special syntax), but used by dunolint to tweak some behavior. Maybe that could be used to select which behavior makes the most sense locally. I should try to locate this and maybe I'll open a discussion with some notes.

I am going to think a bit more about this and get back to you. Thanks!

@mbarbin mbarbin self-assigned this Mar 12, 2025
This was referenced Mar 13, 2025
@mbarbin
Copy link
Owner

mbarbin commented Mar 13, 2025

@raphael-proust your idea of sections is growing on me, I think I'd like to go ahead with it.

I thought of the case where a comment is targeting only its subsequent line, and in this case I suggest the following work-around:

  (* If a comment is meant to target a single library, and that library is meant
     to be excluded from the sorting, it is possible to add an extra comment
     after it to create a new section for the remaining values.

     For example, consider this use-case, which will be incorrectly sorted by
     dunolint as it is: *)
  test
    {|
 (libraries
  ;; foo needs to be first
  foo
  bar
  baz)
|};
  [%expect
    {|
    (libraries
     ;; foo needs to be first
     bar
     baz
     foo)
    |}];
  (* It may be rewritten as follows *)
  test
    {|
 (libraries
  ;; foo needs to be first
  foo
  ;; The rest can be sorted as usual
  bar
  baz)
|};
  [%expect
    {|
    (libraries
     ;; foo needs to be first
     foo
     ;; The rest can be sorted as usual
     bar
     baz)
    |}]; 

Even if the new behavior may not cover all possible cases and scenario, I think it will be at least less inconvenient than what is currently implemented by the current dunolint, so I think this is an improvement.

I created a PR for this, you may act as reviewer if you feel like it. If you are just interested in trying it out, or just giving me the thumbs up that is fine too. Up to you! Thanks a lot for your input.

@raphael-proust
Copy link
Author

I'll check the PR.

I've kept using the tool a bit more and I've mostly encountered three types of comments:

  • end of line (already supported)
  • next line
  • sections

differentiating next line and sections styles probably requires some user input, maybe as part of the dunolint config file

@mbarbin
Copy link
Owner

mbarbin commented Mar 14, 2025

differentiating next line and sections styles probably requires some user input, maybe as part of the dunolint config file

I'm curious what do you think about the workaround I suggested, namely to add an extra comment to change a next-line comment into sections. Too intrusive / inconvenient? The upside I see for this vs editing the config, is that this favors convention over configuration. And perhaps you can argue that for the human reader, this makes things more explicit. I am not very confident because I didn't see enough actual examples of this being used in projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants