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

split assert - avoid split if message, handle comments #52

Merged
merged 1 commit into from
Nov 3, 2022

Conversation

jakkdl
Copy link
Contributor

@jakkdl jakkdl commented Oct 27, 2022

Not triggering on messages was easy, but handling comments was ... hilariously stupid. Turns out the relevant comments can be put in like ... six or seven places? So the code is absolutely stupid and unreadable, but it at least works for all test cases currently written.

TODO:

  • clean up code, can surely deduplicate code handling the left and right side (even though they, of course, differ slightly)
  • add comments (good luck understanding anything atm)
  • write/generate weird tests (some weird combo of lines/comments/statements might put comments in some even more obscure location).

@jakkdl
Copy link
Contributor Author

jakkdl commented Oct 28, 2022

@Zac-HD I think I'm hitting a bug somewhere that's making it impossible to differentiate

assert(
  a # first
  and b # second
)

with

assert(a # first  # second
  and b
)

(which is a pretty common issue, since having multiple #'s on the same line is common with noqa etc. Though one could ship the feature knowing that some comments will be in weird places sometimes)

When running the tests, they both show up with identical SimpleLineStatements in my visitor - but when I parse those strings in my interpreter into either cst.parse_statement or cst.parse_module they are different, as they should be. So something somewhere collapses the two comments in the first one to both be added to updated_node.trailing_whitespace.comment, but they "should" be in updated_node.body[0].operator.whitespace_before.first_line.comment and updated_node[0].rpar[0].whitespace_before.first_line.comment.

I'll try digging into the testing infrastructure to see if the file is read in some janky way or what's going on, but thought I'd raise it since you might have a better idea of what's going on.

@jakkdl
Copy link
Contributor Author

jakkdl commented Oct 28, 2022

Oh derp, it's because shed runs black on it first which reformats it into

assert a and b  # first  # second

@Zac-HD
Copy link
Owner

Zac-HD commented Oct 28, 2022

Ah, right. I'm disinclined to care more about comments than Black itself does, so if you can't reproduce the pattern by using longer variable names I would just skip this case.

@jakkdl
Copy link
Contributor Author

jakkdl commented Oct 28, 2022

once I figured out that Black's been messing with my test cases everything makes a lot more sense, so will have a finished implementation soon. Can't 100% guarantee that comments are in the right places, but I don't wanna throw away any comments at least.

@jakkdl jakkdl force-pushed the split_up_assert branch 2 times, most recently from a6ed8ff to fa1a8b1 Compare October 31, 2022 11:54
@jakkdl jakkdl marked this pull request as ready for review October 31, 2022 11:55
@jakkdl jakkdl force-pushed the split_up_assert branch 2 times, most recently from 4c898cf to 2291638 Compare October 31, 2022 12:35
@jakkdl
Copy link
Contributor Author

jakkdl commented Oct 31, 2022

Done!
Only improvements would be running the refactor before black (which doesn't seem worth it unless other refactors also starts wanting to run before black), and could make it handle

assert a and b; assert c and d

but black will split those up anyway and then they're handled.

@jakkdl
Copy link
Contributor Author

jakkdl commented Oct 31, 2022

The PR did end up madly complicated, and I'm somewhat tempted to want to throw it in the bin - the former implementation straight up eating comments is probably the only thing that makes it worth it. Though if I'd redone it from scratch I'd write a wayy simpler implementation and not give a crap on where the comments go.

Copy link
Owner

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whew, yeah, that certainly is an implementation. Agree that "not eating comments" is important and also that it's not so ugly as to be worth refactoring at this stage. Thanks for the feature; splitting assertions should make it easier to understand what's going on in logs when things fail!

@Zac-HD Zac-HD merged commit 6071209 into Zac-HD:master Nov 3, 2022
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.

None yet

2 participants