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

Split install and configure commands #38

Merged
merged 16 commits into from
Jul 10, 2020
Merged

Split install and configure commands #38

merged 16 commits into from
Jul 10, 2020

Conversation

jayqi
Copy link
Member

@jayqi jayqi commented Jul 7, 2020

Splitting install command

  • Split install into two commands:
    • install installs the post-save hook initialization in the Jupyter config
    • configure creates the .nbautoexport configuration file in a directory
  • When running the configure command, if the post-save hook initialization is absent or out-of-date, there will be a warning printed.
  • Add a new --jupyter-config option to install if user wants to install the post-save hook initialization to a Jupyter config in a nonstandard location.

Other

  • Found a bug from Add new clean, convert, export commands #30 where new clean configuration option was not being passed through install_sentinel. Fixed by changing the interface of install_sentinel to accept a config object instead specifying each option. This reduces likelihood of similar future bugs by reducing the number of places that reproduce the NbAutoexportConfig interface.
  • Replace pkg_resources.parse_version with packaging.version.parse. This is what pkg_resources calls under the hood and is a lighter-weight dependency.
  • Downside of really high test coverage is then it becomes a pain to strictly maintain. Add a .codecov configuration to:
    • Allow up to a 2% decrease in coverage against base
    • Remove the reach graph, which I never found that interpretable but is really visually obnoxious.

Closes #34. I did not unify the naming and opened #37 separately to avoid adding noise to this PR's diff.

@jayqi jayqi requested review from r-b-g-b, pjbull and ejm714 July 7, 2020 06:37
@codecov
Copy link

codecov bot commented Jul 7, 2020

Codecov Report

Merging #38 into master will decrease coverage by 0.1%.
The diff coverage is 96.9%.

@@           Coverage Diff            @@
##           master     #38     +/-   ##
========================================
- Coverage    98.3%   98.1%   -0.2%     
========================================
  Files           8       8             
  Lines         357     381     +24     
========================================
+ Hits          351     374     +23     
- Misses          6       7      +1     
Impacted Files Coverage Δ
nbautoexport/nbautoexport.py 98.2% <96.0%> (-0.7%) ⬇️
nbautoexport/jupyter_config.py 96.5% <100.0%> (+0.1%) ⬆️
nbautoexport/sentinel.py 100.0% <100.0%> (ø)

@@ -44,10 +46,18 @@ def main(
help="Show nbautoexport version.",
),
):
"""Exports Jupyter notebooks to various file formats (.py, .html, and more) upon save,
automatically.
"""Automatically export Jupyter notebooks to various file formats (.py, .html, and more) upon
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah just noticing that typer very nicely adjusts the width of these docstrings when they are printed out in the command line. Now the docstrings can be written with column width of 100 in the source code, but printed out to column width of 80 for the 0.1% of ppl who use ancient monitors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this actually annoyed me recently for a different project where I wanted to put some ASCII art in my help string but typer totally destroyed it with formatting. 😂 😢

Copy link
Collaborator

@r-b-g-b r-b-g-b left a comment

Choose a reason for hiding this comment

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

Amazing stuff! One strange thing about mocking the Jupyter config directory in the tests, then it's good to go.

tests/test_cli_configure.py Show resolved Hide resolved
@jayqi jayqi requested a review from r-b-g-b July 10, 2020 19:44
@jayqi
Copy link
Member Author

jayqi commented Jul 10, 2020

@r-b-g-b I think the issue you had should be fixed. Ready for another look.

Copy link
Collaborator

@r-b-g-b r-b-g-b left a comment

Choose a reason for hiding this comment

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

All set!



def test_configure_no_jupyter_config_warning(tmp_path, monkeypatch):
monkeypatch.setenv("JUPYTER_CONFIG_DIR", str(tmp_path))
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@jayqi jayqi merged commit 58809f6 into master Jul 10, 2020
@jayqi jayqi deleted the install-configure branch July 10, 2020 20:25
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.

Split install command into install and configure
2 participants