Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use self.log.warning instead of warnings.warn #1384

Merged
merged 3 commits into from
Jan 3, 2025

Conversation

yuvipanda
Copy link
Collaborator

warnings.warn sidesteps all the json logging bits we do, and repo2docker will produce non-json output that binderhub then struggles to consume.

@yuvipanda yuvipanda requested review from minrk and manics and removed request for minrk December 8, 2024 08:23
@yuvipanda
Copy link
Collaborator Author

failed: Error during build: Expecting value: line 1 column 1 (char 0)unknown: Exception ignored in: <function Application.__del__ at 0x105855300> is what I get without this from binderhub

@yuvipanda
Copy link
Collaborator Author

Hmm, so the errors aren't actually coming from this - they're coming from somewhere else. Debugging...

warnings.warn sidesteps all the json logging bits we do,
and repo2docker will produce non-json output that binderhub
then struggles to consume.
This actually seems appropriate
@yuvipanda
Copy link
Collaborator Author

I think this should fix the tests!

@manics
Copy link
Member

manics commented Dec 21, 2024

Can you also remove the import warnings? I'm surprised this isn't picked up by one of the linters.

@yuvipanda
Copy link
Collaborator Author

@manics yea i noticed yesterday we don't actually have a flake8 linter here and that means there's actually a ton of unused imports! As well as things like #1392

@manics manics merged commit dd097a2 into jupyterhub:main Jan 3, 2025
19 checks passed
@manics manics added the maintenance Under the hood fixes and improvements label Jan 6, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
maintenance Under the hood fixes and improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants