Skip to content

remote: show default when run "dvc remote list"#10711

Merged
skshetry merged 4 commits into
treeverse:mainfrom
RMCrean:show-default-remote-in-dvc-list
Apr 7, 2025
Merged

remote: show default when run "dvc remote list"#10711
skshetry merged 4 commits into
treeverse:mainfrom
RMCrean:show-default-remote-in-dvc-list

Conversation

@RMCrean

@RMCrean RMCrean commented Mar 26, 2025

Copy link
Copy Markdown
Contributor

Fixes #10708

Hello, I'm new to DVC and this is my first contribution to this repo. Please feel free to make suggestions or edits of course.

I'd also be more than happy to make a documentation PR but I first wanted to check you're happy with the formatting choice I have gone for to show the default remote, as that would affect the documentation.

I have configured it so that the default remote is shown in a similar way to how git branch does it:

Screenshot from 2025-03-26 17-13-24

Thanks!

@codecov

codecov Bot commented Mar 26, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.07%. Comparing base (2431ec6) to head (193bfb9).
Report is 35 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10711      +/-   ##
==========================================
+ Coverage   90.68%   91.07%   +0.39%     
==========================================
  Files         504      504              
  Lines       39795    39948     +153     
  Branches     3141     3158      +17     
==========================================
+ Hits        36087    36383     +296     
+ Misses       3042     2938     -104     
+ Partials      666      627      -39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@skshetry
skshetry self-requested a review March 26, 2025 16:27
Comment thread tests/func/test_remote.py Outdated
Comment thread dvc/commands/remote.py Outdated
RMCrean added 2 commits March 27, 2025 20:40
Now regardless of whether there is a default remote or not, all remotes are equally indented
@RMCrean

RMCrean commented Mar 28, 2025

Copy link
Copy Markdown
Contributor Author

Hello, I think this is ready for a 2nd round of review now. I have also opened a corresponding PR for an update to the documentation

Thanks!

Comment thread dvc/commands/remote.py Outdated
Comment thread dvc/commands/remote.py Outdated
Comment thread dvc/commands/remote.py Outdated
@shcheklein

Copy link
Copy Markdown
Contributor

@RMCrean @skshetry question: (disregard this if you discussed this already). Will introducing a new column with an optional value (* or empty space / tab) make it harder to use unix tools to parse the output (things like cut -f 2 -d " ") ? And btw, some workflow might already depend on this. Should we put the * after the name, url for example? Or introduce a symbol (e.g. - for non default remotes at least). WDYT?

@RMCrean

RMCrean commented Mar 28, 2025

Copy link
Copy Markdown
Contributor Author

Thanks @shcheklein, Yes @skshetry brought up this potential problem to me in this code review comment here. I put some alternatives ways that I could think of doing it in that thread if you want to take a look. Perhaps option 2 (default specified after the other rows) would be best to as that would be unlikely to break any existing code that would rely on something like e.g. cut -f 2 d " "

- Default remote is indicated at end of the output and colored green
- Remotes no longer reordered to show default first
- Test updated to reflect changes and remove need for function escape_ansi.
@RMCrean

RMCrean commented Apr 4, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for the suggestions and advice, I've added a new commit to implement idea 2 which I think is now ready for review. I decide to not change the ordering or the remotes (to put the default first) like I had initially done for the sake of keeping the code simpler. Of course, happy to reorder if preferred.

Here is a preview of the changes:
Screenshot from 2025-04-04 15-51-21

Regarding the failed test, my understanding is this is not related to this PR?

@skshetry
skshetry merged commit e455ee6 into treeverse:main Apr 7, 2025
@skshetry

skshetry commented Apr 7, 2025

Copy link
Copy Markdown
Collaborator

@RMCrean, thank you for contributing. 🙂

@RMCrean

RMCrean commented Apr 7, 2025

Copy link
Copy Markdown
Contributor Author

Awesome! I'll update the dvc docs PR this evening likely 😄

@RMCrean
RMCrean deleted the show-default-remote-in-dvc-list branch April 7, 2025 07:28
@NedJWestern

Copy link
Copy Markdown

Thanks!!

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.

Show default in dvc remote list

4 participants