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

⬆️ Support nbdime v4 (drop <4 support) #62

Merged
merged 3 commits into from
Nov 28, 2023

Conversation

amotl
Copy link
Contributor

@amotl amotl commented Nov 28, 2023

About

@amotl amotl changed the title Update to nbdime4 🔧 MAINTAIN: Update to nbdime4 Nov 28, 2023
Copy link
Owner

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

Team work makes the dream work!

You need to add ("py:class", "nbdime.diffing.config.DiffConfig"), to the nitpick_ignore in the docs/source/conf.py

@amotl amotl force-pushed the update-to-nbdime4 branch from b4c97b5 to ad66b31 Compare November 28, 2023 18:47
@amotl amotl requested a review from chrisjsewell November 28, 2023 18:49
@chrisjsewell chrisjsewell changed the title 🔧 MAINTAIN: Update to nbdime4 ⬆️ Support nbdime v4 (drop <4) Nov 28, 2023
@@ -41,6 +41,7 @@
"_pytest": ("https://doc.pytest.org/en/latest/", None),
# "PIL": ("http://pillow.readthedocs.org/en/latest/", None),
"nbclient": ("https://nbclient.readthedocs.io/en/latest/", None),
"nbdime": ("https://nbdime.readthedocs.io/en/latest/", None),
Copy link
Owner

Choose a reason for hiding this comment

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

but now you should be able to remove the nbdime items in nitpick_ignore 😅

Copy link
Contributor Author

@amotl amotl Nov 28, 2023

Choose a reason for hiding this comment

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

Thanks. I just tried, but it doesn't work: Same error from Sphinx as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also tried adding ("py:class", "DiffConfig"): ("py:class", "nbdime.diffing.config.DiffConfig") to intersphinx_aliases, but it does not make any difference either.

Copy link
Owner

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

cheers

pyproject.toml Outdated
@@ -31,7 +31,7 @@ dependencies = [
"importlib-metadata~=6.0;python_version<'3.10'",
"importlib-resources~=5.0;python_version<'3.9'",
"nbclient~=0.5.10",
"nbdime",
"nbdime<5,>=4",
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
"nbdime<5,>=4",
"nbdime>=4",

actually I think remove the upper-pinning, because otherwise if/when v5 comes out, pip could start pulling in older versions of this package (since they have no pinning)

Copy link
Contributor Author

@amotl amotl Nov 28, 2023

Choose a reason for hiding this comment

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

I will be happy to follow your suggestion.

However, may I ask if that isn't a good thing for stability reasons that only pytest-notebook has the authority to define compatible versions of dependencies? On the other hand, I know that is usual practice not to introduce too many pinnings on software which is actually a library, and rather make it compatible with different versions of dependency packages, in order not to trip dependent software too much. pytest-notebook may qualify as such a package, right?

From a naive perspective, without the upper-pinning, users may run into the same situation we had with the leap to version 4 when version 5 will be published?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will be happy to follow your suggestion.

Fixed.

Copy link
Owner

Choose a reason for hiding this comment

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

its always a tricky subject, but for now I think I'll stick with no pinning and look at that later

@amotl amotl force-pushed the update-to-nbdime4 branch from 49dfcfe to 84f7166 Compare November 28, 2023 19:08
@amotl amotl requested a review from chrisjsewell November 28, 2023 19:09
Copy link
Owner

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

cheers

@chrisjsewell chrisjsewell changed the title ⬆️ Support nbdime v4 (drop <4) ⬆️ Support nbdime v4 (drop <4 support) Nov 28, 2023
@chrisjsewell chrisjsewell merged commit ca71405 into chrisjsewell:master Nov 28, 2023
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