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

Remove ipython_genutils as a dependency #586

Merged
merged 7 commits into from
Oct 19, 2023

Conversation

Carreau
Copy link
Member

@Carreau Carreau commented Sep 29, 2023

This vendors the code.

There is still 1 place using ipython_genutils (ensure_dir_exists), but basically it is only called if you call export_html(..., inline=False), which I can't find anywhere. So if you are willing to remove support for html export with non-inline html, then you can drop ipython_genutils dependency.

@ccordoba12 ccordoba12 added this to the 5.4.5 milestone Sep 30, 2023
Copy link
Collaborator

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @Carreau for your help with this! Some minor comments, the rest looks good to me.

qtconsole/comms.py Outdated Show resolved Hide resolved
qtconsole/completion_plain.py Show resolved Hide resolved
qtconsole/completion_html.py Show resolved Hide resolved
qtconsole/util.py Outdated Show resolved Hide resolved
qtconsole/util.py Show resolved Hide resolved
qtconsole/util.py Show resolved Hide resolved
qtconsole/util.py Outdated Show resolved Hide resolved
qtconsole/util.py Show resolved Hide resolved
qtconsole/util.py Outdated Show resolved Hide resolved
qtconsole/util.py Outdated Show resolved Hide resolved
@ccordoba12 ccordoba12 changed the title remove some of the last vestiges of ipython_genutils Remove some of the last vestiges of ipython_genutils Sep 30, 2023
@ccordoba12 ccordoba12 changed the title Remove some of the last vestiges of ipython_genutils Remove some of the last vestiges of ipython_genutils Sep 30, 2023
@ccordoba12
Copy link
Collaborator

So if you are willing to remove support for html export with non-inline html, then you can drop ipython_genutils dependency.

I agree with that. Could you do it in this PR as well?

@Carreau
Copy link
Member Author

Carreau commented Sep 30, 2023 via email

@ccordoba12
Copy link
Collaborator

Great! Thanks @Carreau!

qtconsole/util.py Outdated Show resolved Hide resolved
qtconsole/util.py Outdated Show resolved Hide resolved
Carreau and others added 3 commits October 1, 2023 09:15
qtconsole/util.py Outdated Show resolved Hide resolved
@Carreau
Copy link
Member Author

Carreau commented Oct 17, 2023

Going through my list of opened PRs, is there anything more I can do here ?

Thanks !

@ccordoba12
Copy link
Collaborator

So if you are willing to remove support for html export with non-inline html, then you can drop ipython_genutils dependency.

I agree with that. Could you do it in this PR as well?

I think this is still unaddressed. I mean, I thought you were going to remove our dependency on ipython_genutils as part of this PR.

@Carreau
Copy link
Member Author

Carreau commented Oct 19, 2023

Oh, yeah sorry, I forgot to remove the lines in setup.py and environment.yml, sorry, oversight.

For html export i was wrong about export_html(,.... inline=...) being unused, so I vendored _ensure_dir_exists. Though the dependency should now be gone.

Copy link
Collaborator

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks for your help with this @Carreau!

@ccordoba12 ccordoba12 changed the title Remove some of the last vestiges of ipython_genutils Remove ipython_genutils as a dependency Oct 19, 2023
@ccordoba12 ccordoba12 merged commit 9e7a97b into jupyter:master Oct 19, 2023
21 checks passed
@Carreau
Copy link
Member Author

Carreau commented Oct 19, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants