feat: 🎸 added optional refcount flag #469

mahendraHegde · 2020-02-21T13:30:26Z

Fixes : #464
refcount optional flag added

BREAKING CHANGE: 🧨 No

Description

implemented an option for the --count option flag that gets passed into the git ref command that populates the list of references.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update

Checklist:

I have read the CONTRIBUTING document.
My commits follow the commitizen commit convention
My change requires a change to the documentation.
I have updated the documentation accordingly.

jwu910 · 2020-02-24T16:57:13Z

Just started reviewing :)

jwu910

Thanks for the PR @mahendraHegde please take a look at my comments

@kienD Can you take a look at the implementation and see what you think around the use of options?

jwu910 · 2020-02-27T19:16:01Z

index.js

-app.start(process.argv.slice(2)).catch(error => {
+const args = process.argv
+  .slice(2)
+  .reduce((acc, val) => acc.concat(val.split('=')), []); // support both "key=val" and "key val"


We don't need to communicate the usage of this logic in an inline-comment

I'm also not 100% convinced we need to pre-process the flags right now. Can you explain the need?

it is not necessary, only if you want to provide --count=2 this prepossessing is needed.
--count 2 will work just fine even without the preprocessing.

I often use both the formats while providing args to CLI tools and most tools support it.
I'll remove it , if you think it is not necessary

Adding to the conversation instead of the new commits: I don't think it is necessary in this case with as few arguments as we have. The way we currently have our options for CIO set up, its a very simple statement and will not work if we add more options in the future.

My reasoning for not wanting to add this line in is that when we get around to adding in a second option, this will break and we won't be able to parse options. I had plans to introduce a cli parsing library at that point. Though it would require some organization of this file.

jwu910 · 2020-02-27T19:18:23Z

src/app.js

@@ -58,6 +59,8 @@ export const start = async args => {
    process.exit(0);
  } else if (args[0] === '--reset-config') {
    conf.all = defaultConfig;
+  } else if (args[0] === '--count' && args[1]) {
+    conf.set('refCount', parseInt(args[1]));


This is creating a side effect of the flag option persisting. Is that the intention? I think options being passed should just act as an overriding flag for a single execution. If we need to persist options through the cli instead of editing the config file, then we can create a ticket to check for a persist/save option in the future.

yes it was intentional, I'll modify it for a single execution then.

I agree that this should be for single execution only.

We will need to add error handling as well for cases where the end user uses a flag but does not pass a value or passes an invalid value like... --count one....etc..

@jwu910 @kienD modified it.

jwu910 · 2020-02-27T19:21:12Z

src/utils/git.js

@@ -182,7 +182,7 @@ const getRefs = async () => {
    'for-each-ref',
    `--sort=${conf.get('sort')}`,
    '--format=%(refname)',
-    '--count=500',
+    `--count=${conf.get('refCount')}`,


To build on the comment about not changing the config object. When a user passes the option to override the count, i think we can use that over the options only when present, otherwise we can use the default options.

modified accordingly.

jwu910 · 2020-02-27T19:25:30Z

README.md

+To set `refCount` , start with the flag `--count`
+
+```
+checkit --count 20
+		OR
+checkit --count=20


May not be necessary per my comments later in the review

jwu910 · 2020-03-11T15:31:12Z

Side note before I finish reviewing, can you update your commit message? This is not a bug fix, the changes are still part of a feature addition. Thanks

kienD · 2020-03-11T16:33:05Z

src/app.js

@@ -315,7 +319,7 @@ export const start = async args => {
   * Update current screen with current remote
   */
  const refreshTable = () => {
-    const tableData = state.currentRemote.refs
+    const tableData = ((state.currentRemote && state.currentRemote.refs) || [])


What did you run into that prompted you to add this?

if you give refCount value very low such as 1 , remotes may not have heads and
here the currentRemote will be set to undefined and will result a crash.

I think this is an acceptable error and reason to crash. It only crashes if you attempt to select an item that does not exist--but effectively an edge case

The purpose of Check It Out is to change branches and if any user is using --count=1 there will only be one option to switch to, or no options depending on the first ref returned from the ref getter.

are you suggesting we should let it crash @jwu910 ?

Yes, the app does not crash on start, it crashes when you are selecting nothing essentially. But you wouldn't use CIO in this manner. The function you linked to is being invoked in a try-catch already.

@jwu910 can you review? its been very long

refcount optional flag added BREAKING CHANGE: 🧨 No ✅ Closes: jwu910#464

refCount should only be valid for current execution BREAKING CHANGE: 🧨 No ✅ Closes: jwu910#464

stale · 2020-05-20T15:30:04Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

jwu910 · 2020-05-27T17:22:14Z

@mahendraHegde I'll try to take a look when I get a chance. Been pretty busy lately with everything going on. It'll be a full fresh review though since it's been so long.

/CC: @kienD

stale · 2020-08-02T18:27:08Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2020-08-16T19:48:42Z

This issue has been automatically closed due to inactivity. If this issue has not been resolved, please reopen the issue. Thank you.

mahendraHegde · 2021-05-30T09:04:31Z

no stale

mahendraHegde requested a review from jwu910 as a code owner February 21, 2020 13:30

jwu910 self-assigned this Feb 24, 2020

jwu910 requested changes Feb 27, 2020

View reviewed changes

kienD self-assigned this Mar 2, 2020

jwu910 requested review from jwu910 and kienD March 11, 2020 15:31

kienD reviewed Mar 11, 2020

View reviewed changes

mahendraHegde force-pushed the CIO-464 branch 2 times, most recently from e6d8ad5 to bb61201 Compare March 13, 2020 13:52

mahendraHegde added 2 commits March 21, 2020 20:30

feat: 🎸 added optional refcount flag

b1d4b1f

refcount optional flag added BREAKING CHANGE: 🧨 No ✅ Closes: jwu910#464

feat: 🎸 refCount for current execution

db74933

refCount should only be valid for current execution BREAKING CHANGE: 🧨 No ✅ Closes: jwu910#464

mahendraHegde force-pushed the CIO-464 branch from bb61201 to db74933 Compare March 21, 2020 15:01

stale bot added wontfix This will not be worked on and removed wontfix This will not be worked on labels May 20, 2020

jwu910 requested a review from kienD June 3, 2020 17:56

stale bot added the wontfix This will not be worked on label Aug 2, 2020

stale bot closed this Aug 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 🎸 added optional refcount flag #469

feat: 🎸 added optional refcount flag #469

mahendraHegde commented Feb 21, 2020

jwu910 commented Feb 24, 2020

jwu910 left a comment

jwu910 Feb 27, 2020

jwu910 Feb 27, 2020

mahendraHegde Mar 1, 2020

mahendraHegde Mar 1, 2020

jwu910 Mar 11, 2020

mahendraHegde Mar 12, 2020

jwu910 Feb 27, 2020

mahendraHegde Mar 1, 2020

kienD Mar 2, 2020

kienD Mar 2, 2020

mahendraHegde Mar 11, 2020

jwu910 Feb 27, 2020

mahendraHegde Mar 11, 2020

jwu910 Feb 27, 2020

mahendraHegde Mar 11, 2020

jwu910 commented Mar 11, 2020

kienD Mar 11, 2020

mahendraHegde Mar 12, 2020

jwu910 Mar 12, 2020

mahendraHegde Mar 12, 2020

jwu910 Mar 12, 2020

mahendraHegde Mar 13, 2020

mahendraHegde May 27, 2020

stale bot commented May 20, 2020

jwu910 commented May 27, 2020

stale bot commented Aug 2, 2020

stale bot commented Aug 16, 2020

mahendraHegde commented May 30, 2021

feat: 🎸 added optional refcount flag #469

feat: 🎸 added optional refcount flag #469

Conversation

mahendraHegde commented Feb 21, 2020

Description

Types of changes

Checklist:

jwu910 commented Feb 24, 2020

jwu910 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwu910 commented Mar 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented May 20, 2020

jwu910 commented May 27, 2020

stale bot commented Aug 2, 2020

stale bot commented Aug 16, 2020

mahendraHegde commented May 30, 2021