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

add: commander to optionally provide delete or download along with st… #516

Merged
merged 19 commits into from
Aug 14, 2024

Conversation

YUUU23
Copy link
Contributor

@YUUU23 YUUU23 commented Jul 27, 2024

  • import commander and initialize commander to be used
  • when no command provided, default action is to run main() as before, all prompts will be asked
    • checks if the studyID / participantID passed in through argument are valid, return from main() if not (validating functions originally in prompt functions are pulled out for this purpose)
  • when user runs npm run cli download [study ID] [participant ID] or npm run cli delete [study ID] [participant ID], relative global variables (ACTION, STUDY_ID, PARTICIPANT_ID) are set to what's provided through command line input, skipping those prompts since we have a value assigned to the global variables already
    • A message is also printed out to let user know which action and args are passed in through the command line
  • Tested by interacting with the command line with different combinations

I accidentally pushed the code to the actual feat-v4 branch, but I reverted this commit (now in this PR)! Sorry about this

@YUUU23 YUUU23 requested a review from RobertGemmaJr July 27, 2024 03:15
@YUUU23 YUUU23 linked an issue Jul 27, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Jul 27, 2024

Visit the preview URL for this PR (updated for commit 1cc9e92):

https://ccv-honeycomb--pr516-add-commander-9itykqix.web.app

(expires Wed, 21 Aug 2024 06:47:16 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 4ace1dcea913a952d2a1af84b94a4421bf36e610

@YUUU23 YUUU23 self-assigned this Jul 27, 2024
@YUUU23 YUUU23 added the cli Organization: Issue in regards to the Honeycomb CLI label Jul 27, 2024
Copy link
Member

@RobertGemmaJr RobertGemmaJr left a comment

Choose a reason for hiding this comment

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

Looking great! There's just a couple small changes as far as the code style

cli.mjs Outdated Show resolved Hide resolved
cli.mjs Show resolved Hide resolved
cli.mjs Outdated Show resolved Hide resolved
cli.mjs Outdated
};
// subcollection is programmatically generated, if it doesn't exist then input must not be a valid studyID
const studyIDCollections = await getStudyRef(input).listCollections();
return studyIDCollections.find((c) => c.id === PARTICIPANTS_COL) ? true : invalidMessage;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... I know this is code I wrote but it looks a little funky to me. Are you okay changing it here?

I would return true or false from this function and then in main where you have that console.error I would just call the string itself - console.error("Please enter a valid study from your Firestore database")

cli.mjs Outdated Show resolved Hide resolved
cli.mjs Outdated Show resolved Hide resolved
@RobertGemmaJr
Copy link
Member

Actually @YUUU23 can you test what this PR would look like if merging into v3.4.2? All of the commander stuff with the CLI script should't have to wait until v4 to work but I'm not entirely sure.

@YUUU23 YUUU23 requested a review from RobertGemmaJr July 31, 2024 20:17
@YUUU23 YUUU23 changed the base branch from feat-v4 to feat-v3.4.2 July 31, 2024 20:54
@YUUU23 YUUU23 changed the base branch from feat-v3.4.2 to feat-v4 July 31, 2024 20:54
@YUUU23
Copy link
Contributor Author

YUUU23 commented Aug 1, 2024

Actually @YUUU23 can you test what this PR would look like if merging into v3.4.2? All of the commander stuff with the CLI script should't have to wait until v4 to work but I'm not entirely sure.

It seems like all the forward commits for v4 features would also be on this branch to be merged into 3.4.2. Do you know if it's possible to rebase on a branch with less features that would delete those v4 features 3.4.2 doesn't have? Otherwise, I think I can also move the CLI script code into a new branch based off of 3.4.2 and open a new PR since the CLI code are all in one file.

@RobertGemmaJr
Copy link
Member

To have this in writing - let's leave it as a change for v4 for right now. Eventually we will release the version and we can include a quick write up for bringing the CLI script into older versions of Honeycomb

Copy link
Member

@RobertGemmaJr RobertGemmaJr left a comment

Choose a reason for hiding this comment

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

Looking good! Just some organizational changes from here!

cli.mjs Outdated Show resolved Hide resolved
cli.mjs Outdated Show resolved Hide resolved
cli.mjs Outdated Show resolved Hide resolved
cli.mjs Outdated Show resolved Hide resolved
cli.mjs Outdated
Comment on lines 245 to 250
// helper to check if the given study (input) is in firestore
async function validateStudyFirebase(input) {
// subcollection is programmatically generated, if it doesn't exist then input must not be a valid studyID
const studyIDCollections = await getStudyRef(input).listCollections();
return studyIDCollections.find((c) => c.id === PARTICIPANTS_COL);
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this helper function (along with validateParticipantFirebase) into a new section for validation functions? It looks like some of the comments aren't above the correct functions anymore

cli.mjs Outdated
Comment on lines 269 to 270
/** Prompt the user to enter the ID of a participant on the STUDY_ID study */
// helper to check if the given participant (input) is in firestore under study
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it looks like the comment on line 269 is for the function defined on line 277

cli.mjs Outdated
Comment on lines 260 to 261
const res = await validateStudyFirebase(input);
return !res ? invalidMessage : true;
Copy link
Member

Choose a reason for hiding this comment

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

We're calling .find inside validateStudyFirebase so I would rename this to be studyCollection or something like that? Because the return of the function is really the Firebase study collection or undefined

@YUUU23 YUUU23 requested a review from RobertGemmaJr August 6, 2024 16:32
@RobertGemmaJr RobertGemmaJr requested a review from eldu August 8, 2024 20:17
Copy link
Contributor

@eldu eldu left a comment

Choose a reason for hiding this comment

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

I commented to simplify some logical expressions. Looks like Rob reviewed this quite a bit ! Small changes, otherwise looks good to go !

cli.mjs Outdated Show resolved Hide resolved
cli.mjs Outdated Show resolved Hide resolved
@YUUU23
Copy link
Contributor Author

YUUU23 commented Aug 14, 2024

I commented to simplify some logical expressions. Looks like Rob reviewed this quite a bit ! Small changes, otherwise looks good to go !

Thank you so much for reviewing this!!

@YUUU23 YUUU23 merged commit a7df3ac into feat-v4 Aug 14, 2024
8 checks passed
@YUUU23 YUUU23 deleted the add-commander branch August 14, 2024 06:49
@YUUU23 YUUU23 restored the add-commander branch August 14, 2024 21:49
@YUUU23
Copy link
Contributor Author

YUUU23 commented Aug 15, 2024

Reverted changes
Most recent changes in this PR: #531

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Organization: Issue in regards to the Honeycomb CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add commander to CLI script
3 participants