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 best attempt at fixing trivia #754

Merged
merged 12 commits into from
Aug 7, 2024

Conversation

Bartleby2718
Copy link
Contributor

@Bartleby2718 Bartleby2718 commented Jun 5, 2024

I think this is now ready for review. I had to delete some tests from #160, so I'd appreciate it if anyone could give me specific suggestions on where to improve. I agree that there are some inconsistencies in terms of where I've touched, but I believe I did need all of these to pass all tests.

Thanks in advance!

This fixes #713.

@Bartleby2718
Copy link
Contributor Author

By the way, can anyone assign the ticket and the PR to me?

@manfred-brands
Copy link
Member

By the way, can anyone assign the ticket and the PR to me?

Done.

@Bartleby2718
Copy link
Contributor Author

All tests are passing now.

@manfred-brands manfred-brands self-requested a review June 9, 2024 05:08
Copy link
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

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

Thanks @Bartleby2718 for your contribution.
Your changes and new tests are comprehensive.

There is, as expected, a lot of repeated:

return (actualArgument, constraintArgument.WithTriviaFrom(actualArgument));

I wonder if you can drop all those and only move this to the ClassicModelAssertUsageCodeFix, where you already deal with the Trivia from the first and last argument.

@Bartleby2718
Copy link
Contributor Author

@manfred-brands I've:

  • added some test cases (CodeFixMaintainsReasonableTriviaWithAllArgumentsOnSameLine) to cover Make best attempt at fixing trivia #754 (comment). (For assertions like ClassicAssert.IsEmpty where only one argument is enough, I've added a message argument so that there are more than one arguments, all on the same line.)
  • centralized the trivia-handling logic, allowing me to revert some unnecessary changes

I feel much better about the changes now. Thanks for the suggestions!

Copy link
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

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

@Bartleby2718 I think we are almost there.
The changes to actual CodeFix are far less than before.
Some small comments.

Copy link
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

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

Thanks @Bartleby2718. This looks good now.

@Bartleby2718
Copy link
Contributor Author

@mikkelbu Could you review this one when you get a chance?

@mikkelbu
Copy link
Member

Sorry for the slow answer, but I've had very little spare time the last couple of weeks. I should have time to look at it tonight (in 8-9 hours time)

Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

Looks great @Bartleby2718. I've only added a handful of some minor nit-pick comments, but the overall PR is great. Thanks for the contribution 👍.

@Bartleby2718 Bartleby2718 requested a review from mikkelbu June 28, 2024 03:01
@Bartleby2718
Copy link
Contributor Author

@mikkelbu This should now be ready for another review!

@mikkelbu
Copy link
Member

Sorry about leaving this hanging. I'll take a look at this next weekend.

Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for this huge effort @Bartleby2718. I'll merge this now

@mikkelbu mikkelbu merged commit ab3d365 into nunit:master Aug 7, 2024
6 checks passed
@mikkelbu mikkelbu added this to the Release 4.3 milestone Aug 7, 2024
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.

[bug] Code fix for NUnit2049 places the comma at a wrong place and messes up indentation
3 participants