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.

Sign up for GitHub

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

Jump to bottom

Update isort version in pre-commit config file. #95

Closed
wants to merge 3 commits into from
Closed

Update isort version in pre-commit config file. #95

wants to merge 3 commits into from

Conversation

zoj613
Copy link
Member

@zoj613 zoj613 commented May 3, 2023 •
edited
Loading

Setting up the pre-commit hooks fails due to a bug in isort (see: PyCQA/isort#2083). This commit updates the version to 5.12.0 where this bug was fixed.

closes #94

Thank you for opening a PR!

Here are a few important guidelines and requirements to check before your PR can be merged:

  • There is an informative high-level description of the changes.
  • The description and/or commit message(s) references the relevant GitHub issue(s).
  • pre-commit is installed and set up.
  • The commit messages follow these guidelines.
  • The commits correspond to relevant logical changes, and there are no commits that fix changes introduced by other commits in the same branch/BR. If your commit description starts with "Fix...", then you're probably making this mistake.
  • There are tests covering the changes introduced in the PR.

Don't worry, your PR doesn't need to be in perfect order to submit it. As development progresses and/or reviewers request changes, you can always rewrite the history of your feature/PR branches.

If your PR is an ongoing effort and you would like to involve us in the process, simply make it a draft PR.

@zoj613 zoj613 added bug Something isn't working CI Relates to continuous integration labels May 3, 2023
@zoj613 zoj613 requested review from brandonwillard and rlouf May 3, 2023 15:27
brandonwillard
brandonwillard reviewed May 3, 2023
View reviewed changes
Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

We need to make it so that "Check code style" runs when the pre-commit config changes. Right now, the requisite check is being skipped.

@zoj613
Copy link
Member Author

zoj613 commented May 3, 2023 •
edited
Loading

We need to make it so that "Check code style" runs when the pre-commit config changes. Right now, the requisite check is being skipped.

Is this a hook specified somewhere? I dont see it in the config.

EDIT: nevermind I see you're referring to the github workflow.

@zoj613 zoj613 force-pushed the fix-isort-hook branch from 73646a2 to 6b26bb1 Compare May 3, 2023 16:56
@zoj613
Update isort version in pre-commit config.
6d5994d 
Setting up the pre-commit hooks fails due to a bug in isort (see:
PyCQA/isort#2083). This commit updates the
version to 5.12.0 where this bug was fixed.
@zoj613 zoj613 force-pushed the fix-isort-hook branch from 6b26bb1 to 344fa41 Compare May 3, 2023 17:06
@zoj613 zoj613 force-pushed the fix-isort-hook branch from 344fa41 to 44dda06 Compare May 3, 2023 17:08
@zoj613
Copy link
Member Author

zoj613 commented May 3, 2023

@brandonwillard could you check this again. The test failure is the unrelated DisconnectedInputError that was mentioned earlier.

@zoj613
Pin urllib version in dev requirements file
7a4dd2b 
When building sphinx docs in the CI environment we get this error:
    ImportError: urllib3 v2.0 only supports OpenSSL 1.1.1+, currently the 'ssl'
    module is compiled with OpenSSL 1.0.2n  7 Dec 2017.
    See: urllib3/urllib3#2168

This commit pins urllib to < 2.0.0 so we can avoid this error until the
environment the docs are built on supports uses a newer openssl version.
@zoj613 zoj613 force-pushed the fix-isort-hook branch from 44dda06 to 7a4dd2b Compare May 6, 2023 18:04
@zoj613 zoj613 requested a review from brandonwillard May 6, 2023 18:21
@codecov
Copy link

codecov bot commented May 6, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (47e1191) 100.00% compared to head (7a4dd2b) 100.00%.

Additional details and impacted files 
@@            Coverage Diff            @@
##              main       #95   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          544       544           
  Branches        31        31           
=========================================
  Hits           544       544

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

brandonwillard
brandonwillard reviewed May 6, 2023
View reviewed changes
Copy link
Member

@brandonwillard brandonwillard left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I added a PR that brings this project setup up to speed with the other Aesara projects: #98. Out of necessity, it also covers the pre-commit updates and a different urllib3 fix, so we can close this one.

@zoj613 zoj613 closed this May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandonwillard brandonwillard brandonwillard left review comments

@rlouf rlouf Awaiting requested review from rlouf

Assignees
No one assigned
Labels
bug Something isn't working CI Relates to continuous integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pre-commit hooks setup is failing.
2 participants
@zoj613 @brandonwillard