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

[feat:stash] add stash API for push, apply, pop, drop, list and clean #228 #980 #1878 #1898

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

modesty
Copy link

@modesty modesty commented Apr 9, 2024

I'm adding a new command: stash

  • [✅ ] add as a new file in src/api (and src/commands if necessary)
  • [✅ ] add command to src/index.js
  • [ ✅] update __tests__/test-exports.js
  • [ ✅] create a test in src/__tests__ - test-stash covers 22 test cases
  • [ ✅] document the command with a JSDoc comment
  • [ ✅] add page to the Docs Sidebar website/sidebars.json
  • [ ✅]] add page to the v1 Docs Sidebar website/versioned_sidebars/version-1.x-sidebars.json
  • [ ✅]] if this is your first time contributing, run npm run add-contributor and follow the prompts to add yourself to the README
  • squash merge the PR with commit message "feat: Added 'X' command"

@jcubic
Copy link
Contributor

jcubic commented Apr 9, 2024

Copy link
Contributor

@jcubic jcubic left a comment

Choose a reason for hiding this comment

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

So far looks good, but there are few things:

  • Can you test untracked files if they stay intact?
  • Do you plan to add ref to stash commands? This is part of canonical git where you can pop or apply different stash entry based on ref (e.g.: stash@{21}) we can use index of entry.

src/commands/stash.js Show resolved Hide resolved
src/commands/stash.js Show resolved Hide resolved
src/commands/stash.js Outdated Show resolved Hide resolved
src/commands/stash.js Show resolved Hide resolved
src/utils/walkerToTreeEntryMap.js Outdated Show resolved Hide resolved
src/utils/walkerToTreeEntryMap.js Outdated Show resolved Hide resolved
src/utils/walkerToTreeEntryMap.js Outdated Show resolved Hide resolved
src/utils/walkerToTreeEntryMap.js Outdated Show resolved Hide resolved
src/commands/stash.js Outdated Show resolved Hide resolved
@modesty
Copy link
Author

modesty commented Apr 10, 2024

So far looks good, but there are few things:

  • Can you test untracked files if they stay intact?

Next commit will add three more test cases:

  1. 'stash with untracked files - no other changes'
  2. 'stash with untracked files - with other changes'
  3. 'stash apply with untracked files - with other staged and unstaged changes'
    it'll bring the total test cases to be 26
  • Do you plan to add ref to stash commands? This is part of canonical git where you can pop or apply different stash entry based on ref (e.g.: stash@{21}) we can use index of entry.

yep, it can be added

… correct the codespell error, plus other tweaks
@modestysn
Copy link

modestysn commented Apr 11, 2024

new commit b9aec41 has added ref (the ref index of stash) to apply, pop and drop, also added more tests on untracked files, valid/invalid refIdx, default stash message, etc., total test cases are 34.

__tests__/test-stash.js Outdated Show resolved Hide resolved
__tests__/test-stash.js Outdated Show resolved Hide resolved
Copy link
Contributor

@jcubic jcubic left a comment

Choose a reason for hiding this comment

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

Looks good, only a few minor things in tests that you can simplify

@jcubic
Copy link
Contributor

jcubic commented Apr 11, 2024

I've added comments to the commit, they are not visible on the PR.

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.

None yet

3 participants