Skip to content

Conversation

Akuli
Copy link
Collaborator

@Akuli Akuli commented Aug 29, 2023

There was an indexing bug that added a trailing comma to the wrong line. It happened every time someone added stubs for a new package whose name is alphabetically at the end of the list, and it looked like this. We don't really need indexing spaghetti to deal with commas, since pyright's "JSON" config file supports trailing commas anyway (even though plain old JSON does not).

Tested locally by deleting stubs and adding them back.

@Akuli Akuli changed the title create_baseline_stubs.py: Fix broken trailing comma bug in create_baseline_stubs.py create_baseline_stubs.py: Fix broken trailing comma bug Aug 29, 2023
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

This relies on us remembering to keep a trailing comma for the last element in the list, right? We could edit it manually (which we do from time to time) and forget to keep a trailing comma with the last element, and none of our tests would fail. Then if somebody came along and tried to add stubs for zzzzz, the script would be broken.

We could possibly add a test to check_consistent.py that asserts that the last element in the list has a trailing comma. But that seems like it could be even more complicated than what we have now

@Akuli
Copy link
Collaborator Author

Akuli commented Aug 29, 2023

I have changed this PR to only fix the indexing bug, reverted other changes, and again tested it locally. Even though I still think the code looks like spaghetti, I cannot come up with anything significantly nicer.

@AlexWaygood
Copy link
Member

Even though I still think the code looks like spaghetti

I mean, I don't disagree :)

@Akuli
Copy link
Collaborator Author

Akuli commented Aug 29, 2023

I think I managed to make it look less spaghetti-like, though it is longer than before. I again tested it locally. It now sorts the list every time, and turns out there were a few places where it wasn't sorted correctly. The only downside I can see is that the code is a bit longer than before.

If we agree that this is a good idea, let's change the title of the PR before we merge.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

This is a big readability improvement. Thanks!

Comment on lines +157 to +158
# We assume that all third-party excludes must be at the end of the list.
# This helps with skipping special entries, such as "stubs/**/@tests/test_cases".
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we'd assert this somehow in check_consistent.py, but I can't think of a simple way of doing so, and a complex way of doing so would probably be overkill for this use case. If this assumption becomes invalidated at some point, we can probably just deal with that when the problem arises.

@Akuli Akuli changed the title create_baseline_stubs.py: Fix broken trailing comma bug create_baseline_stubs.py: Improve pyright config file editing Aug 30, 2023
@Akuli Akuli merged commit a785041 into python:main Aug 30, 2023
@Akuli Akuli deleted the trailing-comma-bug branch August 30, 2023 14:43
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