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

Make inline comments stay inline #9

Closed
wants to merge 2 commits into from
Closed

Make inline comments stay inline #9

wants to merge 2 commits into from

Conversation

jvllmr
Copy link
Contributor

@jvllmr jvllmr commented Mar 2, 2023

First of all, thanks for this project. I use it in my project for detecting ignore comments :D

This is my approach for making inline comments stay inline.
I overwrote the traverse to inspect the first item of the body (maybe a comment) and save the lineno to a list if the lines of body parent and comment match.
When visiting a comment I immediately write the comment with two leading spaces instead of writing a newline by using fill.

When adding a setup.py with your package name you can see which packages depend on yours on github. Just a litte bonus :)

@t3rn0
Copy link
Owner

t3rn0 commented Mar 5, 2023

That's a great work you've done here!
Here are some thoughts:

setup.py

As I understand setup.py adds no value here: poetry.lock is well enough for github and its dependency graph (https://docs.github.com/en/code-security/supply-chain-security/understanding-your-software-supply-chain/about-the-dependency-graph#supported-package-ecosystems).

inlined comments work in very specific cases

Your solution works with def, class, if, ... With everything that has body attribute.

However, if we start identifying inlined comments for if-parts we'd better do this for elif/else parts also (#10).
Moreover, having this inlined-regular logic inside traverse method will make the problem impossible to solve for simple expressions like a = 1 # inlined.

I'd compare line numbers in _enhance function and store inline info into Comment.inline (bool) attribute.
I mean, I don't have any concrete solution yet, it's just a thought.

@jvllmr jvllmr closed this by deleting the head repository Apr 17, 2023
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

Successfully merging this pull request may close these issues.

2 participants