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

Introduce StagePath for working with files on stages #1674

Merged
merged 4 commits into from
Oct 17, 2024

Conversation

sfc-gh-turbaszek
Copy link
Contributor

@sfc-gh-turbaszek sfc-gh-turbaszek commented Oct 7, 2024

Pre-review checklist

  • I've confirmed that instructions included in README.md are still correct after my changes in the codebase.
  • I've added or updated automated unit tests to verify correctness of my new code.
  • I've added or updated integration tests to verify correctness of my new code.
  • I've confirmed that my changes are working by executing CLI's commands manually on MacOS.
  • I've confirmed that my changes are working by executing CLI's commands manually on Windows.
  • I've confirmed that my changes are up-to-date with the target branch.
  • I've described my changes in the release notes.
  • I've described my changes in the section below.

Changes description

  1. Renames existing StagePath to StagePathType to avoid ambiguity
  2. Introduce StagePath class representing a file on stage. This class offers Path like operations including joinpath or parent operations.

The next step of refactor will be removing of StagePathParts concepts and then moving query generation to StagePath.

["@repo/branches/main/s1.sql", "@repo/branches/main/a/S3.sql"],
),
(
"@repo/branches/main/a",
"@repo/branches/main/",
"@repo/branches/main",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To align the behavior. The StagePath accepts both inputs (with or without trailing /) but the representation is always without / suffix

@sfc-gh-turbaszek sfc-gh-turbaszek marked this pull request as ready for review October 9, 2024 09:23
@sfc-gh-turbaszek sfc-gh-turbaszek requested review from a team as code owners October 9, 2024 09:23
@sfc-gh-turbaszek sfc-gh-turbaszek marked this pull request as draft October 9, 2024 09:24
@sfc-gh-turbaszek sfc-gh-turbaszek marked this pull request as ready for review October 17, 2024 15:33
@@ -30,7 +30,7 @@

log = logging.getLogger(__name__)

StagePath = PurePosixPath # alias PurePosixPath as StagePath for clarity
StagePathType = PurePosixPath # alias PurePosixPath as StagePath for clarity
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is done to avoid ambiguity

@sfc-gh-turbaszek sfc-gh-turbaszek enabled auto-merge (squash) October 17, 2024 17:02
Copy link
Contributor

@sfc-gh-fcampbell sfc-gh-fcampbell left a comment

Choose a reason for hiding this comment

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

NADE LGTM

@sfc-gh-turbaszek sfc-gh-turbaszek merged commit 06c3876 into main Oct 17, 2024
21 checks passed
@sfc-gh-turbaszek sfc-gh-turbaszek deleted the introduce-stage-path branch October 17, 2024 17:06
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.

3 participants