Skip to content

Conversation

@birgelee
Copy link
Member

@birgelee birgelee commented Feb 1, 2025

No description provided.

@birgelee birgelee requested a review from sciros February 1, 2025 01:25
@sciros
Copy link
Collaborator

sciros commented Feb 1, 2025

Big thanks for enabling these.
Pretty sure Black formatter is going to go ballistic on these long parameterized lines, just FYI. We might even want to wrap them in a # fmt: off and # fmt: on (or whatever it is) so it doesn't do that.

@birgelee
Copy link
Member Author

birgelee commented Feb 1, 2025

@sciros I thought the original lines looked a bit better, but not by much. I would prefer consistency, so I just ran black formatter on the code files. If the integration tests look good to you I would like to proceed.

@sciros
Copy link
Collaborator

sciros commented Feb 1, 2025

Yeah.. I'd have wrapped them in fmt: off/on because this to me is a clear case of rigid formatting rules absolutely butchering readability. Trying to tell whether we're missing a specific test case while having to scroll through a couple of screens' of lines is exactly where rigid, naive formatters completely fall over and should be discarded.
I suspect even the author of the formatter would agree that more than doubling the size of the file means Black is not a good fit for what the code is trying to do.
I wish there were a way to configure Black to not touch specific statements like pytest.mark.parametrize(..) but I haven't found it.

I believe you should revert this change.
Applying the formatter to the rest of the actual code is fine, but what it does to the test parameters is absolutely heinous from a code clarity standpoint.

Copy link
Collaborator

@sciros sciros left a comment

Choose a reason for hiding this comment

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

Please revert application of Black formatter to parametrize lines.

@birgelee
Copy link
Member Author

birgelee commented Feb 1, 2025

Personally I did not object to the formatting as strongly. These test files can get crazy large and I was already primarily using search to get around. Regardless, I agree the original line style was better and I wrapped with fmt on and fmt off. Then I ran the formatter on the rest of the file. @sciros let me know your thoughts.

@sciros
Copy link
Collaborator

sciros commented Feb 1, 2025

Personally I did not object to the formatting as strongly. These test files can get crazy large and I was already primarily using search to get around. Regardless, I agree the original line style was better and I wrapped with fmt on and fmt off. Then I ran the formatter on the rest of the file. @sciros let me know your thoughts.

I think this is a good compromise. Thanks for making the change.

Honestly, for now I am regretting a little bit the incorporation of the Black formatter, given how we've not had any issues with formatting AT ALL up to this point, except for ironically just ones that Black introduced, and having to toss in format on/off switches because of Black's limitations (can't specify statements to ignore, can't apply formatting to parts of files) doesn't exactly contribute to "beautiful Python," but I am hoping that this maybe heads off any potential issues with this being OSS.

Probably I'll take this as a lesson learned to not include it in future Python efforts unless it somehow becomes necessary, and instead tell people to just follow PEP8 (https://pep8.org/) guidelines.

@birgelee birgelee merged commit 5ee79ba into main Feb 1, 2025
1 check passed
@birgelee birgelee deleted the birgelee-integration-testing-http branch February 1, 2025 23:17
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