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

Blacken #177

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Blacken #177

wants to merge 15 commits into from

Conversation

njsmith
Copy link
Member

@njsmith njsmith commented May 10, 2023

I'm working on fixing a bug with connection shutdown, and standarizing the formatting first makes the source easier to work with

@njsmith
Copy link
Member Author

njsmith commented May 10, 2023

to be clear: this is literally just black trio_websocket tests, no configuration, no other edits.

trio.MultiError is deprecated, and for technical reasons involving how
the deprecation is implemented, this means pylint can't see it and
thinks it doesn't exist. It does exist.
@njsmith
Copy link
Member Author

njsmith commented May 10, 2023

OK the last two commits did actually change things in trivial ways to get the lint passing. Still no semantic changes, or even syntactic ones.

@belm0
Copy link
Member

belm0 commented May 10, 2023

Thank you for investigating that problem.

I'm not a fan of blacken formatting*, but the plea I'll make is that making a huge formatting change is not ideal for a project on life support. With a legacy codebase and departed authors, it's critical to be able to dig through code history and understand things-- I'd rather avoid having to navigate through this diff on every blame output going forward.

Would it be reasonable to revisit if the project becomes more actively maintained?

[*] - for example, some of the formatting choices are not friendly to vertical footprint, and I do 100% development on a small laptop having a short ratio display.

@jakkdl
Copy link
Member

jakkdl commented Oct 26, 2023

I was requested to add type hints to this repository, so

Would it be reasonable to revisit if the project becomes more actively maintained?

is now relevant. I would vastly prefer if the code was black-formatted for me to change stuff.

With a legacy codebase and departed authors, it's critical to be able to dig through code history and understand things-- I'd rather avoid having to navigate through this diff on every blame output going forward.

you can tell git blame to ignore certain commits by adding their ID in a file named .git-blame-ignore-revs. We can't do it in this PR (or at least I failed when I tried earlier) due to squashing and merging changing IDs, but can do it in a followup PR.

@@ -11,6 +11,7 @@ test:
$(PYTHON) -m pytest --cov=trio_websocket --no-cov-on-fail

lint:
$(PYTHON) -m black trio_websocket/ tests/ autobahn/ examples/
Copy link
Member

Choose a reason for hiding this comment

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

Hi @jakkdl

I was requested to add type hints to this repository

Would you share some context of that conversation? (gitter link etc.)

Please see the thread here before we discuss type annotation coverage.
#167 (comment)

Would it be reasonable to revisit if the project becomes more actively maintained?

is now relevant

To me, being off life-support maintenance would mean things like 1) there is at least another person that has some history on the project and I can trust to share the load on whatever comes up going forward, 2) there is activity like revising the API, rewriting the implementation or tests, etc. I wouldn't say that adding type annotations or auto formatting means that the project is out of the woods regarding stewardship, and there are aspects of full type annotations that make maintenance harder.

Regards,

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Thank you! We'll look into possible alternative libraries that are under more active support and reconsider whether to use trio-websockets. If we don't find any great alternatives, I could go with a more minimal approach and just write separate stub files that needn't touch the existing code at all, and if you don't want the extra maintenance burden of them we can just stick them in an entirely separate repo.

@belm0 belm0 mentioned this pull request Jun 6, 2024
2 tasks
@CoolCat467
Copy link
Member

Current test failures are from old code trying to use MultiError, which no longer exists.

@CoolCat467
Copy link
Member

Turns out I just missed adding some pylint ignores, as it knows that trio doesn't have MultiError anymore but the code in question was meant for handling said older versions of Trio.

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.

4 participants