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

Updates to 1. zsh autocomplete script and 2. slow link type warning message #2003

Merged
merged 13 commits into from
May 24, 2019
Merged

Updates to 1. zsh autocomplete script and 2. slow link type warning message #2003

merged 13 commits into from
May 24, 2019

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented May 14, 2019

  • Have you followed the guidelines in our
    Contributing document?

  • N/A Does your PR affect documented changes or does it add new functionality
    that should be documented? If yes, have you created a PR for
    dvc.org documenting it or at
    least opened an issue for it? If so, please add a link to it.


@jorgeorpinel jorgeorpinel changed the title Update to Zsh autocomplete script WIP: Update to Zsh autocomplete script May 14, 2019
@efiop efiop requested a review from a user May 14, 2019 19:44
scripts/completion/dvc.zsh Outdated Show resolved Hide resolved
scripts/completion/dvc.zsh Outdated Show resolved Hide resolved
scripts/completion/dvc.zsh Outdated Show resolved Hide resolved
scripts/completion/dvc.zsh Outdated Show resolved Hide resolved
@shcheklein
Copy link
Member

shcheklein commented May 14, 2019

@jorgeorpinel why does your commit history look strange? rebase?

@efiop
Copy link
Contributor

efiop commented May 14, 2019

@shcheklein that is how merges look :) That is why I usually advise people to

git remote add upstream https://github.com/iterative/dvc
git fetch upstream
git rebase upstream/master
git push -f

when they are submitting PRs 🙂

@shcheklein
Copy link
Member

@shcheklein that is how merges look :) That is why I usually advise people to

it depends on your workflow with merge, right? :) If you always use a new feature-branch from master or if you don't squash commits it should not include commits that are already merged.

@efiop
Copy link
Contributor

efiop commented May 14, 2019

@shcheklein Well, in general merge can result in stuff like that. You are lucky if you get "fast-forward"(which is effectively a no-conflict rebase), but with "rebase" you always get a nice-looking PRs only with your commits 🙂

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

It is hard to review sort + changes 😅

@ghost
Copy link

ghost commented May 14, 2019

@jorgeorpinel , besides sorting and adding the new commands, did you change anything?

@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented May 15, 2019

@efiop thanks for letting me know about hidden commands, I'll remove those comments in a bit...

@shcheklein its just merged from upstream/master. Should I be rebasing always? I'm not too used to rebasing but I'm happy to adopt

@MrOutis yes I should have sorted in one commit and changed in another, will keep in mind in the future. The actual updates are just to the descriptions of the commands, to match the current help output of each one.

@jorgeorpinel jorgeorpinel changed the title WIP: Update to Zsh autocomplete script Updates to 1. zsh autocomplete script and to 2. slow cache link type warning message May 15, 2019
@jorgeorpinel jorgeorpinel changed the title Updates to 1. zsh autocomplete script and to 2. slow cache link type warning message Updates to 1. zsh autocomplete script and 2. slow link type warning message May 15, 2019
@jorgeorpinel
Copy link
Contributor Author

@MrOutis @shcheklein the new completion script is done now and I tried it manually on my system, please review.

@jorgeorpinel
Copy link
Contributor Author

Also, after rebasing upsteam/master this history includes my changes to git diff (to output short commit hashes) again but those were already merged in #1906. I'm not sure why that happened.

@efiop efiop requested review from pared and a user May 20, 2019 18:24
@pared
Copy link
Contributor

pared commented May 20, 2019

I like the change, thought I think we should consider some changes when handling message strings in general, @jorgeorpinel, had to push another change updating the test warning message. As @Suor pointed in #1942 this is not desired workflow. Maybe we should create a task for creating separate file with warning and error messages?

@efiop
Copy link
Contributor

efiop commented May 20, 2019

@pared Not sure it is worth a separate file. It is not like you are going to reuse those other than in the test. Defining it as a constant somewhere in the same file seems good enough.

@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented May 20, 2019

A constant in the appropriate class or file may be enough like Ruslan points out but it also sounds like a good practice to extract strings to its own file(s) if we're thinking about i18n in the future. M 2¢

UPDATE: Do we need an issue for this?

@efiop
Copy link
Contributor

efiop commented May 20, 2019

@jorgeorpinel @pared already added #2026 , but forgot to reference 🙂

@pared
Copy link
Contributor

pared commented May 21, 2019

Sorry for not referencing it here

@jorgeorpinel
Copy link
Contributor Author

All good. OK so what's missing for this PR? Can it be merged? 😃

@efiop
Copy link
Contributor

efiop commented May 21, 2019

@jorgeorpinel Let's wait for @MrOutis to review.

dvc/scm/git/__init__.py Outdated Show resolved Hide resolved
dvc/scm/git/__init__.py Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

dvc/scm/git/__init__.py Outdated Show resolved Hide resolved
@jorgeorpinel
Copy link
Contributor Author

Done. That docstring Args wrapping was an accident 😅

ghost
ghost previously requested changes May 23, 2019
dvc/scm/git/__init__.py Outdated Show resolved Hide resolved
dvc/scm/git/__init__.py Outdated Show resolved Hide resolved
jorgeorpinel and others added 12 commits May 24, 2019 01:29
- Docstring for Git class constructor
- Renames self.git for self.repo throughout module (involves several test changes)
- Better doc and logical refactor in Git._get_diff_trees
- `dvc diff` now outputs short SHA strings (Git._get_diff_trees) - hard coded to 7 length str
  (involves some test changes in tests/func/test_diff.py)

For #1902
- Reorganizes commands in alphabetical order;
- Updates the command dscriptions according to their current usage (-h) output;
- Adds comments for MISSING command entries
Co-Authored-By: Ruslan Kuprieiev <[email protected]>
Co-Authored-By: Ruslan Kuprieiev <[email protected]>
Co-Authored-By: Ruslan Kuprieiev <[email protected]>
Co-Authored-By: Ruslan Kuprieiev <[email protected]>
dvc/scm/git/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Thanks! 🎉

@efiop efiop dismissed ghost ’s stale review May 24, 2019 21:00

stale

@efiop efiop merged commit ce83231 into iterative:master May 24, 2019
@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented May 28, 2019

Thank you! I noticed some of the - options in some commands don't come up in auto-complete though :p may need to revise again soon (probably same for bash).

p.s. see also #1924

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.

4 participants