Fix missing struct field separators under certain conditions by cassaundra · Pul...

Copy link

Contributor

@ytmimi if/when you get a chance could you take a look at this? At some point we had a different fix for this that can be found over on the 2.0 branch, so would like to compare this approach vs. that one. I also feel like you may have possibly done some somewhat similar work recently and thought you may be interested in reviewing

ytmimi commented on Dec 30, 2021

Sure thing! I'll try to find the fix on the 2.0 branch to compare it to and will hopefully be able to get the review done in the next few days. I'll let you know if I run into any issues.

ytmimi commented on Jan 2

I managed to track down #4201, which I believe is the PR that fixes this issue on the 2.0 branch. Both fixes are very similar and involve passing additional information to rewrite_aligned_items_inner. The previous PR passes a slice of the remaining fields to rewrite_aligned_items_inner, while the new proposal just passes a boolean. I happen to prefer the newer solution since it's a little more flexible in case there are other scenarios that we need to force trailing separators.

I'm happy with the changes as they are! @cassaundra If you have time would it be possible to add test cases with trailing_comma=Always and test cases where struct_field_align_threshold=0. Since these are the two configuration options that you need to reproduce the issue it would be great if we had test cases that fully covered their interaction in this scenario.

cassaundra commented on Jan 3

Thank you for the review! I will add those test cases soon.

cassaundra commented on Jan 15 •

Apologies for the late response.

I've added the tests as requested, hopefully providing better coverage between struct_field_align_threshold and trailing_comma. The tests are located in tests/{source,target}/issue-4791/ and are named as follows:

zero non-zero

always (default) trailing_comma.rs

never no_trailing_comma.rs buggy.rs

ytmimi commented on Jan 18

Thanks for adding those additional test cases! I think we now have the test coverage we need. @calebcartwright, is there anything else you'd like to see added? If not, I'm happy to move forward with these changes.

calebcartwright commented on Jan 29

Changes LGTM! @cassaundra would you mind adding test files for #4928 too?

cassaundra commented 6 days ago

Sorry again for the late reply! It's been a crazy month. Those tests have now been added.

ytmimi commented 5 days ago

@cassaundra Thanks for including that additional test!