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

Add Viewer mode #642

Merged
merged 15 commits into from
Dec 1, 2020
Merged

Add Viewer mode #642

merged 15 commits into from
Dec 1, 2020

Conversation

robintw
Copy link
Collaborator

@robintw robintw commented Nov 27, 2020

🧰 Issue

#610

🚀 Overview:

Adds a new Pepys Viewer app, which only provides access to the View Data commands within Pepys Admin. This can be called from the command-line using --viewer, or from two new Start Menu shortcuts: Pepys Viewer and Pepys Viewer (training mode).

🤔 Reason:

For some users, we wish to let them use Pepys-Admin to view data, but not to have access (or require training in) the rest of the Admin operations.

🔨Work carried out:

  • Added --viewer shortcut, and made it load just the viewing interface
  • Added Windows Start Menu shortcuts
  • Tests pass

🖥️ Screenshot

Screen Shot 2020-11-27 at 13 46 11

Screen Shot 2020-11-27 at 13 46 48

Confirmations

  • I have chosen reviewers for my PR.
  • I have chosen an appropriate label for the PR.
  • I have completed the mandatory sections of this document.
  • I have deleted any unused sections.

📝 Developer Notes:

This depends on #624, and that should be merged first. I have already merged in the branch for #624 into this branch, as they were both modifying lots of the same files and I needed to sort out all the conflicts. Hopefully Github will do the sensible thing and only show the new commits here, once #624 is merged.

Added --viewer command line argument
Changed ViewDataShell to behave slightly differently depending
on whether it is being called as admin or viewer
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@IanMayo IanMayo added the interactive_review Triggers an interactive review via Binder label Nov 27, 2020
@github-actions
Copy link

github-actions bot commented Nov 27, 2020

🚀 Interactive demo available via Binder logo.

Note: The Jupyter notebook browser will open in the demos folder. See comments above for which notebook to use,
otherwise use the _Default_Demo.ipynb notebook.

@codecov
Copy link

codecov bot commented Nov 27, 2020

Codecov Report

Merging #642 (c809a63) into develop (1d48e6f) will decrease coverage by 0.03%.
The diff coverage is 87.50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #642      +/-   ##
===========================================
- Coverage    99.10%   99.07%   -0.04%     
===========================================
  Files           64       64              
  Lines         6054     6071      +17     
===========================================
+ Hits          6000     6015      +15     
- Misses          54       56       +2     
Impacted Files Coverage Δ
pepys_admin/view_data_cli.py 94.65% <70.00%> (-2.15%) ⬇️
pepys_admin/cli.py 92.68% <100.00%> (+6.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8295409...f6ec0f5. Read the comment docs.

@robintw robintw requested review from BarisSari and IanMayo and removed request for BarisSari November 30, 2020 09:26
@robintw robintw marked this pull request as ready for review November 30, 2020 10:49
@IanMayo
Copy link
Member

IanMayo commented Nov 30, 2020

Thanks @robintw - could you have a think about what changes should be made to read the docs please?

I'm pretty sure we should include the new command line parameters, but there may be a need for some higher level content - such as introducing the concept of the viewer.

@IanMayo
Copy link
Member

IanMayo commented Nov 30, 2020

@robintw - it looks like there is an issue with Create Deployment. Maybe prompt_toolkit has moved.

@robintw
Copy link
Collaborator Author

robintw commented Nov 30, 2020

This create_deployment issue seemed to be an issue with installing black using the latest version of pip. Pip have released a new version (20.3) today, which switches to a new package resolver which is a lot more strict than the previous version. Pip was checking the metadata for the black package it had downloaded and raised an error as some of the metadata versions didn't match. This only seems to occur on the embedded version of Python we use in the Windows deployment, not on a normal Windows Python.

This has been temporarily solved by pinning black to an older version in requirements_dev.txt. This older version (only two versions ago) has a Python wheel available on PyPI which means pip can just download that and use it, without needing to build it itself - skipping the part that causes problems.

I've raised an issue on the black Github repo at psf/black#1847. I have a suspicion they're going to tell me it's pip's problem, so I might end up raising an issue for pip too. I'll keep an eye on it all and let you know what happens.

As part of investigating this, I've noticed that failures in running the actual create_deployment.bat script aren't being picked up properly by Github Actions as a failure - I think the return codes from the various scripts aren't propagating properly. I'll add a separate issue to fix that.

@robintw
Copy link
Collaborator Author

robintw commented Nov 30, 2020

Argh, ignore that - pinning to an older black version means black wants to reformat all of our files! I'll work on this more tomorrow.

Copy link
Member

@IanMayo IanMayo left a comment

Choose a reason for hiding this comment

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

I ❤️ the viewer logo.

@IanMayo IanMayo merged commit 88cb344 into develop Dec 1, 2020
@IanMayo IanMayo deleted the viewer-mode branch December 1, 2020 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interactive_review Triggers an interactive review via Binder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants