Skip to content

Preserving comment-only lines? #16

Closed

Description

@di

Currently this project only preserves comments when they're on a line with syntax, otherwise they are removed:

pyproject.toml:

[project]
# A comment-only line
name = "something"  # This is the name
# Another comment
$ python -m pyproject_fmt pyproject.toml
--- pyproject.toml

+++ pyproject.toml

@@ -1,4 +1,2 @@

 [project]
-# A comment-only line
 name = "something"  # This is the name
-# Another comment

Seems like it would make sense to either:

  • a) preserve all comments
  • b) remove all comments

My personal opinion is that a) would be ideal!

Activity

  1. gaborbernat commented on Mar 25, 2022

    @gaborbernat
    Member

    Yeah, a is the expected behaviour.

  2. tusharsadhwani commented on Mar 25, 2022

    @tusharsadhwani

    I'll try and fix this.

  3. tusharsadhwani commented on Mar 25, 2022

    @tusharsadhwani

    entries = {i.key: (i, v) for (i, v) in body if isinstance(i, Key)}

    This seems to be the bug. entries gets rid of all non key-value lines from body, i.e. comment lines are deleted in this step.

    Herein lies the ambiguity: What part of the code do the comments belong to? The line of code below the comment? The one above? Something else entirely?

    It is certainly fixable, but we should define a clear answer for this.

  4. tusharsadhwani commented on Mar 25, 2022

    @tusharsadhwani

    Possible convention:

    • If a comment has a key-value line below it and nothing/whitespace above it, it belongs to that line.
    • If a comment has a key-value line above it and nothing/whitespace below it, it belongs to that line.
    • If a comment has a key-value line both above it AND below it, we assume it belongs to the line below.

    Examples:

    [test]
    # this comment belongs to the line below
    x = 10
    
    y = 20
    # this comment belongs to the line above
    # this comment also belongs to the line above
    
    # this comment belongs to the line below
    z = 30
    # this comment belongs to the line below
    w = 40
    # this comment belongs to the line above
  5. gaborbernat commented on Mar 25, 2022

    @gaborbernat
    Member

    I think all comments up to a key belong to that key. However, comments that don't have any key after them (comments at the end of file) should be kept.

  6. tusharsadhwani commented on Mar 25, 2022

    @tusharsadhwani

    @gaborbernat what about this:

    [options.entry_points]
    # some comment about the project in general
    
    b = 20
    a = 10

    when you sort the entry points, you don't want the top level comment to be dragged with b = 20

  7. gaborbernat commented on Mar 25, 2022

    @gaborbernat
    Member

    I don't agree, I think it should be dragged 🤔if one wants generic comments could be before the table

  8. tusharsadhwani commented on Mar 25, 2022

    @tusharsadhwani

    is this the desired outcome? (whitespace being removed as well)

    [options.entry_points]
    a = 10
    # some comment about the project in general
    b = 20
  9. gaborbernat commented on Mar 25, 2022

    @gaborbernat
    Member

    More like:

    [options.entry_points]
    a = 10  # some comment about the project in general
    b = 20

    If fits in one line, otherwise:

    [options.entry_points]
     # some comment about the project in general
    a = 10
    b = 20

    I can see an argument for putting comments after the key it belongs to too. Need to have a think. One of those two. Generally, whitespace should be collapsed IMHO.

  10. tusharsadhwani commented on Mar 25, 2022

    @tusharsadhwani

    I see, that makes sense. Something got misunderstood before.

    When you say "all comments up to a key belong to that key", in this case:

    [foo]
    # bar
    
    x = 10
    y = 20

    that would mean # bar belongs to x = 10. Which doesn't seem right to me, # bar here is more like its own little block, unrelated to any key.

    Which I believe is what you're thinking of as well.

  11. tusharsadhwani commented on Oct 25, 2023

    @tusharsadhwani

    @gaborbernat honestly i don't think I'd be able to get back to this. i have an unfinished PR as a starting point if someone wants to take it up.

  12. amotl commented on Dec 21, 2023

    @amotl
    Contributor

    Hi there. Apologies if it might be unrelated, but at least it is also about preserving comments or not, and how, so I wanted to reference this other occurrance where pyproject-fmt is tripping us a bit. Nothing serious though, thanks for your great work!

  13. adamtheturtle commented on Jan 21, 2024

    @adamtheturtle
    Contributor

    I hit the exact same issue as @amotl . I'm happy to make a new issue if preferred.

    What I want:

    [project.optional-dependencies]
    dev = [
        "check-manifest",
        # Comment about why we pin doc8 to this version
        "doc8==0.11.2",
    ]

    The comment is associated with doc8.
    pyproject-fmt moves the comment next to "check-manifest".

  14. gaborbernat commented on May 3, 2024

    @gaborbernat
    Member

    FYI, I've been playing around to moving the project to use the taplo https://taplo.tamasfe.dev AST parser and formatter that would allow this - https://github.com/gaborbernat/pyproject-fmt-rust; will likely take another few weeks for this to happen but help there is welcome.

  15. edgarrmondragon commented on May 3, 2024

    @edgarrmondragon
    Contributor

    FYI, I've been playing around to moving the project to use the taplo https://taplo.tamasfe.dev AST parser and formatter that would allow this - https://github.com/gaborbernat/pyproject-fmt-rust; will likely take another few weeks for this to happen but help there is welcome.

    @gaborbernat do we need an issue to track where the migration stands in terms of feature parity?

  16. gaborbernat commented on May 3, 2024

    @gaborbernat
    Member

    The test suite itself is the tracker. Currently, if you check, the CI is failing as there's no feature parity.

  17. gaborbernat commented on May 3, 2024

    @gaborbernat
    Member
    image some progress
  18. gaborbernat commented on May 8, 2024

    @gaborbernat
    Member
    Screenshot 2024-05-07 at 10 08 58 PM

    getting closer

  19. gaborbernat commented on May 10, 2024

    @gaborbernat
    Member
  20. namurphy commented on May 10, 2024

    @namurphy
    Contributor

    Awesome! Thank you for doing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workinghelp wantedExtra attention is needed

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions