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

1792 Add docstrings to upgrade.py #2512

Merged
merged 12 commits into from
Jun 19, 2024

Conversation

arjxn-py
Copy link
Contributor

Should fix #1792

This PR adds file-level documentation strings for upgrade.py

What does this implement/fix?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

@arjxn-py arjxn-py changed the title 1792 Add docstrings to upgrage.py 1792 Add docstrings to upgrade.py Jun 15, 2024
@viniciusdc
Copy link
Contributor

Looking great, we will be reviewing this during the week, we're in the process of releasing, so if this does not enter in this current release, it will be in the next 🚀

@viniciusdc viniciusdc added the needs: review 👀 This PR is complete and ready for reviewing label Jun 17, 2024
Copy link
Contributor

@dcmcand dcmcand left a comment

Choose a reason for hiding this comment

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

Hi @arjxn-py, thanks so much for your contribution. There are some minor style changes needed, but once that is done, we can get this merged!

src/_nebari/upgrade.py Outdated Show resolved Hide resolved
src/_nebari/upgrade.py Outdated Show resolved Hide resolved
src/_nebari/upgrade.py Show resolved Hide resolved
src/_nebari/upgrade.py Outdated Show resolved Hide resolved
@dcmcand dcmcand added area: documentation 📖 Improvements or additions to documentation needs: changes 🧱 Review completed - some changes are needed before merging and removed needs: review 👀 This PR is complete and ready for reviewing labels Jun 18, 2024
@arjxn-py
Copy link
Contributor Author

arjxn-py commented Jun 19, 2024

Thanks for the constructive review @dcmcand, i've made the suggested changes, hope I interpreted it correctly.
I'd like to know how can i run pre-commit hook correctly locally before committing my changes.
I'm doing pre-commit run which is returning me -

fix end of files.....................................(no files to check)Skipped
trim trailing whitespace.............................(no files to check)Skipped
check json...........................................(no files to check)Skipped
check yaml...........................................(no files to check)Skipped
check toml...........................................(no files to check)Skipped
check that executables have shebangs.................(no files to check)Skipped
codespell............................................(no files to check)Skipped
black................................................(no files to check)Skipped
ruff.................................................(no files to check)Skipped
isort................................................(no files to check)Skipped
Terraform fmt........................................(no files to check)Skipped

@arjxn-py
Copy link
Contributor Author

In addition to that, I see that python==3.10 & python==3.11 tests are failing because the @override decorator is actually available in the typing module starting from Python 3.12. See also - https://docs.python.org/3/whatsnew/3.12.html#pep-698-override-decorator-for-static-typing

So should I use typing_extensions instead?

@arjxn-py arjxn-py requested a review from dcmcand June 19, 2024 08:22
@dcmcand
Copy link
Contributor

dcmcand commented Jun 19, 2024

In addition to that, I see that python==3.10 & python==3.11 tests are failing because the @override decorator is actually available in the typing module starting from Python 3.12. See also - https://docs.python.org/3/whatsnew/3.12.html#pep-698-override-decorator-for-static-typing

So should I use typing_extensions instead?

ah, good catch, yes, use typing_extensions

@dcmcand
Copy link
Contributor

dcmcand commented Jun 19, 2024

Thanks for the constructive review @dcmcand, i've made the suggested changes, hope I interpreted it correctly. I'd like to know how can i run pre-commit hook correctly locally before committing my changes. I'm doing pre-commit run which is returning me -

fix end of files.....................................(no files to check)Skipped
trim trailing whitespace.............................(no files to check)Skipped
check json...........................................(no files to check)Skipped
check yaml...........................................(no files to check)Skipped
check toml...........................................(no files to check)Skipped
check that executables have shebangs.................(no files to check)Skipped
codespell............................................(no files to check)Skipped
black................................................(no files to check)Skipped
ruff.................................................(no files to check)Skipped
isort................................................(no files to check)Skipped
Terraform fmt........................................(no files to check)Skipped

You should be able to run it. Use pre-commit run --all-files

@arjxn-py
Copy link
Contributor Author

You should be able to run it. Use pre-commit run --all-files

Yes, thanks it works 🚀

@arjxn-py
Copy link
Contributor Author

arjxn-py commented Jun 19, 2024

Oh Conda build failing, I just noticed that conda has the latest release of version 4.11.0 for typing_extensions, on it to fix it.
Edit: Done.

Copy link
Contributor

@dcmcand dcmcand left a comment

Choose a reason for hiding this comment

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

Thanks for the quality contribution @arjxn-py 🚀

@dcmcand dcmcand merged commit 7396df8 into nebari-dev:develop Jun 19, 2024
12 checks passed
@arjxn-py arjxn-py deleted the 1792-add-docstrings-to-upgrage.py branch June 19, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: documentation 📖 Improvements or additions to documentation needs: changes 🧱 Review completed - some changes are needed before merging
Projects
Development

Successfully merging this pull request may close these issues.

[DOC] Add doc string to upgrade.py
3 participants