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

More of a Q: Empty lines #67

Open
MementoRC opened this issue Dec 5, 2022 · 4 comments
Open

More of a Q: Empty lines #67

MementoRC opened this issue Dec 5, 2022 · 4 comments

Comments

@MementoRC
Copy link
Contributor

My preference is that the empty lines in the original code remains (as much as possible) in the modified code. There is a small change that enables this in the PreciseUnparser, however, it seems to be by design (connected comments vs non-connected in the test_ast.py) that empty lines are not considered. Is it better to create a new PreciseEmptyLinesUnparser? or simply it is a matter of preference and no need to PR since my needs are met in my fork?

@isidentical
Copy link
Owner

Hey MementoRC, thanks a lot fo the PR(s) and the questions! I think the empty lines can also be preserved, but we need to be careful (a lot of tests of scenarios, especially with inner replaces) I'll try to take a look at them as soon as possible (hopefully tomorrow)

@MementoRC
Copy link
Contributor Author

MementoRC commented Dec 14, 2022

@isidentical For the InsertAfter and InsertBefore, I like to have a separating line sometimes, is it the right way to add and InserBeforeWithSeparator, ... classes to the actions? or is there a mean to add options to the existing classes? The new class only has the following addition:

        # Adding extra separating line
        replacement.append(lines._newline_type)

@isidentical
Copy link
Owner

@isidentical For the InsertAfter and InsertBefore, I like to have a separating line sometimes, is it the right way to add and InserBeforeWithSeparator, ... classes to the actions? or is there a mean to add options to the existing classes? The new class only has the following addition:

I think it makes sense to have an option in both of them that takes separator: str | None (for InsertAfter, that's between the target <-> new node, and for InsertBefore it is between the new node <-> target).

@MementoRC
Copy link
Contributor Author

This does not seem to be possible with the dataclass, that is for LazyInsertAfter since InsertAfter inherits from it and would have to define it first in the list of arguments passed to the instantiation - Or at least, this is above my padawan-level ;P
Hints? Help?

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