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

Compatibility with jupyter_core version 5+ #255

Closed
wants to merge 7 commits into from

Conversation

sizhky
Copy link
Contributor

@sizhky sizhky commented Sep 8, 2024

Specifically improves compatibility with jupyter environments that use ipykernels like jupyter-lab

#230

@sizhky sizhky changed the title Compatibility with jupyter notebooks 5+ Compatibility with jupyter_core version 5+ Sep 8, 2024
@cool-RR
Copy link
Owner

cool-RR commented Sep 8, 2024

Looks good.

  1. I think that the if after your code should be an elif.
  2. Can you show a screenshot of before and after this change in the Jupyter shell?
  3. Are you reasonable sure you're not breaking anything else?
  4. If there happens to be a Windows version of /var/folders/, it'll be nice to include it too. Not a dealbreaker.
  5. Change \d in the regular expression to [0-9].

@sizhky
Copy link
Contributor Author

sizhky commented Sep 8, 2024

  • I think that the if after your code should be an elif.
  • Can you show a screenshot of before and after this change in the Jupyter shell?
  • Are you reasonable sure you're not breaking anything else?
  • If there happens to be a Windows version of /var/folders/, it'll be nice to include it too. Not a dealbreaker.
    I'm not in a position to test this.
  • Change \d in the regular expression to [0-9].
image

@cool-RR
Copy link
Owner

cool-RR commented Sep 8, 2024

Good job. All done and ready for merging?

@sizhky
Copy link
Contributor Author

sizhky commented Sep 8, 2024

Yep!

@sizhky
Copy link
Contributor Author

sizhky commented Sep 9, 2024

Anything still pending?

@cool-RR
Copy link
Owner

cool-RR commented Sep 9, 2024

My grandma always told me to let PRs chill in the fridge overnight before merging, they taste better that way.

Merged and released. Would you care to install PySnooper 1.2.1 using pip (or any other package manager) and test whether it works fine?

@cool-RR cool-RR closed this Sep 9, 2024
@sizhky
Copy link
Contributor Author

sizhky commented Sep 9, 2024

yeah it's working

@cool-RR
Copy link
Owner

cool-RR commented Sep 9, 2024

Thank you. Good job.

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