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

Various changes #96

Merged
merged 10 commits into from May 12, 2023
Merged

Various changes #96

merged 10 commits into from May 12, 2023

Conversation

brl0
Copy link
Contributor

@brl0 brl0 commented Apr 25, 2023

These changes are broken out from the result of some long running tinkering I started a couple of years ago. To help keep the review process reasonable, I am trying to strike a balance of not creating too many small PRs while also not creating overly large PRs. Along those lines I am willing to break out or combine change sets in whatever manner is most comfortable for review, just let me know.

Changes in this PR:

  • Improve some error messages by including the string value of the path so it is clear what failed.
  • Refactored __new__, extracting a method for creating paths from other path objects to improve readability and clarity.
  • Cleaned up notebook for clarity and brevity, also added table of fsspec supported filesystems.
  • Added custom warning to make it easier to filter, also added simple function to do so.
  • Fixed bug causing incorrect rglob results (top level glob matches were not included).
  • GitHub Actions
    • Only run on PRs and pushes to main, to fix duplicate runs.
    • Don't fail fast, so that multiple failures can be reviewed after each run.
    • Run on "3.11" rather than "3.11-dev".

Changes split out for later:

  • Refactor tests and fixtures for improved isolation.
    • Make it easier and safer to allow for tests to be run live.
    • Reduce inadvertent test interaction and flakiness.
    • Eventually try to use UPath's great test suite directly from other fsspec filesystem projects to improve coverage.
  • Migrate to hatch, stop using legacy setup metadata format.
  • Update and consolidate dev tools and test dependencies.
  • New nox tasks and minor changes for convenience.
  • Add, configure, and apply pre-commit for consistent code formatting.

I'd be eager for any feedback or suggestions. Thanks!

Copy link
Collaborator

@ap-- ap-- left a comment

Choose a reason for hiding this comment

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

Thank you so much for your contribution!

I went through and made some comments.
Regarding the other changes you mentioned in the PR text, I would suggest to open a new issue, so we can discuss how to move forward.

Let me know what you think,
Andreas

upath/errors.py Outdated Show resolved Hide resolved
upath/errors.py Outdated Show resolved Hide resolved
upath/errors.py Outdated Show resolved Hide resolved
upath/core.py Show resolved Hide resolved
upath/core.py Show resolved Hide resolved
upath/implementations/cloud.py Outdated Show resolved Hide resolved
upath/registry.py Outdated Show resolved Hide resolved
notebooks/examples.ipynb Show resolved Hide resolved
notebooks/examples.ipynb Show resolved Hide resolved
.github/workflows/python.yml Outdated Show resolved Hide resolved
This was referenced Apr 26, 2023
@ap--
Copy link
Collaborator

ap-- commented Apr 26, 2023

Hi @brl0,

I opened two new issues for the remaining items you mentioned in the PR text.

Cheers,
Andreas 😃

@brl0
Copy link
Contributor Author

brl0 commented Apr 28, 2023

Hey @ap--,

Thanks so much for your timely and thorough review, I enjoy learning from feedback and really appreciate your time.

I made some changes and responded to some of your comments and would love to hear any more of your thoughts.

brl0 and others added 2 commits May 8, 2023 09:35
Co-authored-by: Andreas Poehlmann <ap--@users.noreply.github.com>
@brl0
Copy link
Contributor Author

brl0 commented May 8, 2023

@ap--,

I thought I'd follow up to see if my changes were acceptable, and to see if you had any more feedback on this PR.

Copy link
Collaborator

@ap-- ap-- left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the comments and adding the tests ❤️

There's one small change left, and then we're good to merge this.

upath/core.py Outdated Show resolved Hide resolved
@brl0
Copy link
Contributor Author

brl0 commented May 9, 2023

@ap--,

Thanks again for your review. I have made the requested changes.
I'm curious about your thoughts on my comment in #90.

Once this gets merged, I'll put together a PR with the refactored tests.

@brl0 brl0 requested a review from ap-- May 9, 2023 19:51
@ap--
Copy link
Collaborator

ap-- commented May 12, 2023

Thank you @brl0 ❤️

This is a great improvement. I'll merge this now and will prepare a new release over the weekend.

Also thank you so much for your patience 🙇 It's been a few busy weeks over here...
When we move forward with the other improvements let's break them up into individual pieces when it makes sense to speed up the process as much as possible.

@ap-- ap-- merged commit a07ae58 into fsspec:main May 12, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants