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

Allow caps in dirname #420

Merged
merged 3 commits into from
Oct 5, 2023
Merged

Allow caps in dirname #420

merged 3 commits into from
Oct 5, 2023

Conversation

dacbd
Copy link
Contributor

@dacbd dacbd commented Sep 25, 2023

No description provided.

@dacbd dacbd temporarily deployed to pypi September 25, 2023 21:25 — with GitHub Actions Inactive
@dacbd dacbd temporarily deployed to pypi September 25, 2023 21:32 — with GitHub Actions Inactive
@pmrowla
Copy link
Contributor

pmrowla commented Sep 26, 2023

related: #403

@dberenbaum dberenbaum mentioned this pull request Oct 4, 2023
Merged
@dberenbaum
Copy link
Contributor

See also https://github.com/iterative/studio/pull/7844.

LGTM. I added #424 on top of this to support the same in model names.

@dberenbaum
allow uppercase model names (#424)
d4d53e0 
On top of #420, this adds support for uppercase model names.
@dberenbaum
Copy link
Contributor

@dacbd @pmrowla Product questions are resolved. Is there anything blocking marking this as ready and merging?

@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2023 •
edited
Loading

Codecov Report

All modified lines are covered by tests ✅

Files Coverage Δ
gto/constants.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@dberenbaum dberenbaum temporarily deployed to pypi October 4, 2023 18:27 — with GitHub Actions Inactive
@dberenbaum dberenbaum mentioned this pull request Oct 4, 2023
Closed
@dberenbaum
Copy link
Contributor

See iterative/dvc#9821 (comment). Should we force tags to lowercase?

@pmrowla
Copy link
Contributor

pmrowla commented Oct 5, 2023

Since the tags can include directory paths, forcing them to lowercase would break behavior if the user has capitalized directory names and they are on a platform with case-sensitive filesystems.

The main issue here is that the dir path part of the regex disallows a lot of valid path characters (which was raised in #403 (comment))

@dacbd dacbd self-assigned this Oct 5, 2023
@dacbd dacbd marked this pull request as ready for review October 5, 2023 01:11
@dacbd dacbd merged commit 701fa68 into main Oct 5, 2023
17 checks passed
@dacbd dacbd deleted the dacbd-patch-1 branch October 5, 2023 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dberenbaum dberenbaum dberenbaum approved these changes

@shcheklein shcheklein shcheklein approved these changes

Assignees

@dacbd dacbd

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dacbd @pmrowla @dberenbaum @codecov-commenter @shcheklein