-
Notifications
You must be signed in to change notification settings - Fork 662
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
[MRG] Refactor argparse #129
Conversation
dd8145f
to
5d7de06
Compare
okay rebased so that the diff looks more reasonable. Should I continue for the other files? |
@jasmainak nice job! thanks for pushing this through :-) I'm +1 on doing something like this with the other main CLI functions as well. I think it'll make things more modular and easier to test! |
e5ab087
to
e243950
Compare
@choldgraf you might want to start reviewing already so that the work is distributed and the PR doesn't stay open for too long and I become sad. maybe @emdupre can also review? :) |
@@ -242,17 +219,15 @@ def upgrade_book(): | |||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of try
statements ...
errors pass silently if you have them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though in this case we're capturing errors and displaying a custom error message at the end if we run into the errors, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an reasonable try/except
block to me, but would be really interested in how you think it could be improved @jasmainak !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was my priors commenting rather than the posterior :-P But wouldn't you hide problems in the upgrade if you use such a block? Imagine a user has a problem upgrading. When you ask them for the traceback, they won't have one because it's been caught and instead you will get the print statement -- which is not so useful for debugging.
@choldgraf codecov is not working. Are you sure you enabled it? |
hey @jasmainak, sorry, I've been at the Jupyter annual community meeting all week in Washington DC, so haven't gotten any work done. I have a flight back to SF this evening, will try giving this a review on the flight if I can get wifi |
Reviewing this is on my to-do list for this weekend -- Looking forward to having a chance to sit down with it ✨ Thanks for the work so far, @jasmainak ! |
(also just a note @jasmainak , in general I don't review PRs if they have "WIP" in the title unless explicitly asked to. To me WIP means "please don't look closely yet because I might change stuff in the near future") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks much cleaner @jasmainak , thanks so much! It all seems pretty reasonable to me, I like splitting off the "commands" into its own module. I had a couple small comments in there, but I think as long as tests are happy then I am happy with this step.
I won't "approve" since this is still marked as WIP, happy to look at something specific if you like.
@@ -242,17 +219,15 @@ def upgrade_book(): | |||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though in this case we're capturing errors and displaying a custom error message at the end if we run into the errors, no?
Haha okay. But now you have been explicitly asked to :p I fear that it will
be too much reviewing work to do in one go. But if you prefer that way,
just wait one more day :) The WIP for me says - don’t merge. And reviews
are welcome from the moment I make a PR ;)
On Fri 15 Mar 2019 at 18:26, Chris Holdgraf ***@***.***> wrote:
(also just a note @jasmainak <https://github.com/jasmainak> , in general
I don't review PRs if they have "WIP" in the title unless explicitly asked
to. To me WIP means "please don't look closely yet because I might change
stuff in the near future")
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#129 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/APHioqt3A4GWZvuv_HAelIP_ZmPuZ_79ks5vXB4sgaJpZM4bnczr>
.
--
Sent from my iPhone
|
From my first pass above, it didn't seem like there was too much complexity being added, no? You're basically rearranging what is there rather than adding something new yeah? I think this makes it much easier to review because you're basically just doing a much better job of building a CLI than I did in my first pass :-) |
e243950
to
80e5bc1
Compare
Co-Authored-By: jasmainak <jasmainak@users.noreply.github.com>
This LGTM! @emdupre do you wanna take a look before we merge? |
I can look tomorrow, but if you're happy with everything @choldgraf feel free to merge now :) |
okay, so I think I'm done here. I didn't touch please do test it locally though. There aren't many unit tests in the repo -- hence I highly recommend testing it before merging :) |
ok cool, so |
are you holidaying in France again????! :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great ! Very easy to read, now 👍 I had a few questions about docstrings and how we're using argparse, but really excited about this ! ✨
jupyter_book/commands/create.py
Outdated
############################################### | ||
# Default values and arguments | ||
|
||
args = parser.parse_args(sys.argv[2:]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate of L51 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of writing to args
can we just set dest
for each of the options ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm ... does dest
allow you to avoid parser.parse_args
? I thought dest
is inferred from the option string so you don't need to specify it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I shouldn't have used dest
. I was thinking something like:
args = parser.parse_args(sys.argv[2:])
new_book(**vars(args))
Also, multiple variables are set from toc
here, and it looks like we have verbose
twice !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's really neat! But here, we have a couple of variables that shouldn't be passed as is. Perhaps something to iterate in future PRs?
Also, that was a good catch! The fact that the tests pass indicate that the tests should be improved.
@@ -242,17 +219,15 @@ def upgrade_book(): | |||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an reasonable try/except
block to me, but would be really interested in how you think it could be improved @jasmainak !
toc = yaml.load(ff.read()) | ||
|
||
# Drop divider items and non-linked pages in the sidebar, un-nest sections | ||
toc = _prepare_toc(toc) | ||
|
||
############################################################################### | ||
################################################ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@choldgraf just trying to slowly make it more pep8 compliant :) So we can remove the ignore for the 80 line rule eventually ...
Thanks @emdupre for the review! I addressed all of them save one. I'll reply to that specific comment in a moment. @choldgraf this PR made me realize that you also need an API page. Different functions take different variable names and there are some inconsistencies. But I would leave it for another PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried pulling @jasmainak 's branch and the books built well for me + tests pass, I agree with @emdupre that this is much more understandable, and the code LGTM. I'm +1 on merge, feel free to hit the green button @emdupre
Sorry I'm so slow to respond on these -- I'm finishing up writing my dissertation proposal so things are a little busy at the moment 😞 I made one more comment but feel free to merge without addressing if we want to iterate later. I think this is a huge improvement and don't want to hold it up because of my lateness ! Thanks again @jasmainak ✨ |
parser.add_argument("--custom-css", default=None, | ||
help="A path to a CSS file that defines some custom CSS rules for your book") | ||
parser.add_argument("--custom-js", default=None, | ||
help="A path to a JS file that defines some custom CSS rules for your book") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@choldgraf this seemed like an awkward help statement to me. But I let it be as such because I don't know the inner workings of jupyter-book yet. Why would a JS file define CSS rules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, this should be "JS" not "CSS"!
help="A path to a JS file that defines some custom CSS rules for your book") | |
help="A path to a JS file that defines some custom JavaScript code for your book") |
@jasmainak if you accept my suggestion let's merge this thing! :-) |
I can't accept. Github tells me I can't accept a suggestion on a deleted line :) |
ok, I opened #146 to keep track of the CLI problems :-)
thanks for the awesome patch @jasmainak - should make things easier to improve and iterate on! |
This PR builds on top of #126 (to avoid rebasing troubles) -- it's why the diff is large. Once that is merged into master, the diff wouldn't look so bad. It's separating the argument parsing from the actual nuts and bolts of jupyter-book. This has two advantages that I can clearly see:
I just did it for build.py at the moment, but if it's useful I can do it for all the files.