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

Pathlib everywhere #1773

Merged
merged 18 commits into from
Jul 20, 2023
Merged

Pathlib everywhere #1773

merged 18 commits into from
Jul 20, 2023

Conversation

pmeier
Copy link
Member

@pmeier pmeier commented May 2, 2023

Reference Issues or PRs

Closes #1713.

What does this implement/fix?

7k951s

We are Buzz Lightyear here and Woody is someone stuck on Python < 3.4. As the title implies, this replaces os.path with pathlib.Path everywhere in the nebari package except for the templates. LMK if we want pathlib there as well.

  • 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?

Any other comments?

@pmeier pmeier linked an issue May 2, 2023 that may be closed by this pull request
@pmeier
Copy link
Member Author

pmeier commented May 9, 2023

There is one file missing that I need to refactor. After that it should only be getting the tests green. I'm not sure if I have time this week to address this. But it is still on my backlog.

@pmeier
Copy link
Member Author

pmeier commented May 10, 2023

/bot run tests

@github-actions
Copy link

Contributor Tests Triggered by @pmeier

return value


CONFIG_PATH_OPTION: Path = typer.Option(
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only refactor I've made. Since the configuration file and output path is used by multiple subcommands, I've defined them once at the top. This avoids duplication in the code and makes it easier to maintain.

@pmeier pmeier marked this pull request as ready for review May 10, 2023 20:33
@Adam-D-Lewis
Copy link
Member

@pmeier Looks like the Kubernetes tests fail https://github.com/nebari-dev/nebari/actions/runs/4940974457/jobs/8833155254 and also some merge conflicts

@iameskild
Copy link
Member

@pmeier it looks like we will need update this PR due to the recent changes to the API. Is this something you'll have time for?

@pmeier
Copy link
Member Author

pmeier commented Jun 12, 2023

I'm OOO for the better part of the next two weeks. Will pick it up after. Feel free to change the labels on the PR to indicstr that if necessary.

@iameskild
Copy link
Member

Hi @pmeier, I know we've gone back and forth on this PR but do you have time to wrap this up? It's also worth noting that many of the changes in here will likely be overridden by the work coming from the extension-mechanism. Perhaps it would be better to wait until that work is complete...

@pmeier
Copy link
Member Author

pmeier commented Jul 17, 2023

Unless there is some blocker I failed to see, I promise I finish it before tomorrows community meeting.

@pmeier pmeier requested a review from iameskild July 18, 2023 14:40
@pmeier pmeier added needs: review 👀 This PR is complete and ready for reviewing and removed status: in progress 🏗 This task is currently being worked on labels Jul 18, 2023
Copy link
Member

@iameskild iameskild left a comment

Choose a reason for hiding this comment

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

Beautiful :)

@iameskild iameskild merged commit 08a8b82 into nebari-dev:develop Jul 20, 2023
8 checks passed
@pmeier pmeier deleted the pathlib branch July 20, 2023 06:00
@costrouc costrouc mentioned this pull request Aug 3, 2023
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:developer-experience 👩🏻‍💻 needs: review 👀 This PR is complete and ready for reviewing
Projects
Development

Successfully merging this pull request may close these issues.

[ENH] - also expand ~ in paths [ENH] - Use pathlib everywhere
4 participants