-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add support of project in disks #2960
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2960 +/- ##
=======================================
Coverage 73.63% 73.63%
=======================================
Files 70 70
Lines 12500 12518 +18
Branches 2303 2306 +3
=======================================
+ Hits 9204 9218 +14
- Misses 2975 2977 +2
- Partials 321 323 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
neuro-cli/src/neuro_cli/disks.py
Outdated
if isinstance(disk, URL): | ||
disk_str = disk.parts[-1] | ||
else: | ||
disk_str = disk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please explain why we dropped resolve_disk
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, originally the intent was to simplify this logic. When I started to explain possible cases, I find out that my reasoning was wrong and we still need this method for cases when we have disk URLs in full form or with the owner.
Reverted in c381a1e.
@@ -27,7 +30,11 @@ def formatter(uri: URL) -> str: | |||
owner, _, rest = path.partition("/") | |||
else: | |||
owner = owner_or_org | |||
if owner == username: | |||
# TODO (Y.S.): remove this check and use project_name parameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we assume right away that there is no owner in URIs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could, originally was planning to avoid those merge-conflicts which will be caused by this change. made changes in 866ebfe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
closes #2951