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

95 openpyxl selenium imports #96

Merged
merged 5 commits into from
Feb 5, 2024
Merged

95 openpyxl selenium imports #96

merged 5 commits into from
Feb 5, 2024

Conversation

sqr00t
Copy link
Contributor

@sqr00t sqr00t commented Feb 2, 2024

Update some more dependencies

This PR closes the issues: #95
This PR also partially addresses: #89

Description

  1. Updates minimum version requirements for Selenium(>=4.17.2) and PyYAML(>=6.0.1)
  2. Implement error handling for excel methods requiring openpyxl backend
  3. Small changes to type hints for path parameters in function signatures in viz.altair.saving.py
  4. Adds __init__.py to package modules

Checklist:

  • I have followed Contributor Guidelines
  • I have updated the documentation to include any new functionality I have added or modified
  • I have written unit tests for any new functionality I have added - not applicable
  • I have ran and passed all tests locally with >> pytest
  • I have checked that all tests ran successfully on the Github after I pushed

Copy link
Contributor

@j-gillam j-gillam left a comment

Choose a reason for hiding this comment

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

Everything works for me! I trialled an excel example and saved a few altair plots.

@@ -70,26 +74,32 @@ def _save_png(
)


def _save_html(fig: Chart, path: os.PathLike, name: str, scale_factor: int):
def _save_html(
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny tiny aesthetic comment so please feel free to ignore, but I noticed you separated out parameters into different lines for _save_svg and _save_png but haven't here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pre-commit linting checks made these changes to separate lines if the line exceeds a (PEP8?) character count (should be ~80?)

Copy link
Contributor

@j-gillam j-gillam left a comment

Choose a reason for hiding this comment

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

Worked perfectly in the example I have been testing it on. I have one comment around where the try excepts could be. But it isn't necessary for functionality, so happy for you to merge!

@@ -0,0 +1,9 @@
from nesta_ds_utils.loading_saving.gis_interface import _gis_enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to move the gis_interface try except into here as well? Instead of importing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd keep less logic in init.py , the imports are needed for defining the functions in gis_interface anyways. So I'd tend towards importing by traversing up the directory tree to the top level features_enabled dict

@sqr00t sqr00t merged commit 2e5af4e into dev Feb 5, 2024
3 checks passed
@sqr00t sqr00t deleted the 95_openpyxl_selenium_imports branch February 5, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants