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

[isort] First implementation based on code_prettify #1204

Merged
merged 1 commit into from Feb 1, 2018
Merged

[isort] First implementation based on code_prettify #1204

merged 1 commit into from Feb 1, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jan 21, 2018

See #1202 for discussion. Actually I didn't add keyboard shortcuts because I can't find any
combination that works on my machine.

@jcb91
Copy link
Member

jcb91 commented Jan 21, 2018

I can't find any combination that works on my machine

When you say 'works', in what sense have the ones youo've tried not worked? Or do you mean that you just can't find any more spare defaults?


All options are provided by the [KerneExecOnCells library](kernel_exec_on_cell.js). There are a few nbextension-wide options, configurable using the [jupyter_nbextensions_configurator](https://github.com/Jupyter-contrib/jupyter_nbextensions_configurator) or by editing the `notebook` section config file directly. The options are as follows:

- `2to3.add_toolbar_button`: Whether to add a toolbar button to transform the selected cell(s). Defaults to `true`.
Copy link
Member

Choose a reason for hiding this comment

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

these options in the readme need 2to3 replacing with isort

input_type: text
default: 'Sort imports with isort'

- name: isort.show_alerts_for_errors
Copy link
Member

Choose a reason for hiding this comment

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

since you've caught all exceptions in the library function, this option might be better of not listed, since it won't actually do anything

Actually I didn't add keyboard shortcuts because I can't find any
combination that works on my machine.
@ghost
Copy link
Author

ghost commented Jan 22, 2018

Thanks for your review @jcb91, I've amended the changes.

@jcb91
Copy link
Member

jcb91 commented Jan 23, 2018

Great, looks good to me 👍 @benjaminabel Could you also submit an equivalent PR to the upstream jfbercher/code_prettify?

@jcb91
Copy link
Member

jcb91 commented Jan 23, 2018

actually, no need, I'll do that...

@ghost
Copy link
Author

ghost commented Jan 23, 2018

I didn't know existence about this repository. Why don't you include it as a git submodule?

@jcb91
Copy link
Member

jcb91 commented Jan 23, 2018

Why don't you include it as a git submodule?

It more or less is already in practice, since they share history. I think you're probably right, it'd make sense to make this a submodule explicitly. Thoughts @jfbercher?

@jfbercher
Copy link
Member

@jcb91 Sorry for being late to reply.
Actually, I think it could be easier to bring everything in jupyter_contrib_nbextensions ; then I would delete the upstream demo or mention that development goes on here.

@jcb91
Copy link
Member

jcb91 commented Jan 24, 2018

Sorry for being late to reply

No problem at all! This is nobody's job, after all ;)

Actually, I think it could be easier to bring everything in jupyter_contrib_nbextensions; then I would delete the upstream demo or mention that development goes on here

Oh, ok! I don't really mind either way, so happy to go with whatever works for you 👍 Feel free to close #1208

@jcb91
Copy link
Member

jcb91 commented Feb 1, 2018

Ok, I'm going to merge this in here, assuming this copy to be the definitive one, as suggested above.

@jcb91 jcb91 merged commit 39049a6 into ipython-contrib:master Feb 1, 2018
@ghost
Copy link
Author

ghost commented Feb 2, 2018

Thanks for merging. Do we now add an action process_all_cells_as_one to the kernel_exec_on_cell object?

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