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

remove strict check for branch in curated json #161

Merged
merged 6 commits into from
Nov 26, 2024

Conversation

technophile-04
Copy link
Collaborator

Description:

For CLI to work internally we don't need branch strict check. Also removed the strict check for descriptionl.

@technophile-04
Copy link
Collaborator Author

Ahh I see @damianmarti what you were saying because we had this in :

acc[ext.branch] = {

and branch could be optional you were telling us we default to name right?

I think maybe we should add a new field extensionFlag because in above line [ext.branch] is what CLI tracks when when user passes extension with -e we were assuming that it would be equal to branch name.

So now difference b/w name and extensionFlag is name could be human readable name(only useful for scaffoldeth.io) and extensionFlag is codename thing which user passes as -e flag to CLI. If extension doesn't have extensionFlag by default it it's extension flag is branch name.

// json file type
type Extension = {
  extensionFlag?: string; // optional if not present we use branch
  branch?: string; // optional if not present we use extensionFlag
  repository: string;
  // use full for scaffoldeth.io
  description: string;
  version?: string; // (optional) if not present we default latest
  name?: string; // (optional) if not present we use the branch name as default (name might be useful to nicely display challenges names)
  // github: can be inferred by joining repositry + branch (scaffoldeth.io is requiring this for displaying link)
  // installCommand can inferred using repository + branch + version
}[];

@rin-st
Copy link
Member

rin-st commented Nov 25, 2024

Ahh I see @damianmarti what you were saying because we had this in :

Wanted to ask the same thing)

What do you think about making extensionFlag required field for curated extensions? So it will be explicit and we won't need to suppose is it branch or not

Also thinking about better name but cant find something better for now). extensionFlagValue, extensionCliValue, cliValue? Make this field name, and rename current name to title?

@damianmarti
Copy link
Member

Ahh I see @damianmarti what you were saying because we had this in :

Wanted to ask the same thing)

What do you think about making extensionFlag required field for curated extensions? So it will be explicit and we won't need to suppose is it branch or not

Also thinking about better name but cant find something better for now). extensionFlagValue, extensionCliValue, cliValue? Make this field name, and rename current name to title?

maybe shortName?

@technophile-04
Copy link
Collaborator Author

technophile-04 commented Nov 26, 2024

What do you think about making extensionFlag required field for curated extensions?

Yess makes sense! Updated it at 9fbe052 and also changed the field name to extensionFlagValue.

Make this field name, and rename current name to title?

maybe shortName?

The data submitted by builders in buidlguidl app has name field in json which correspondence to Card title in scaffoldeth.io . That's the reason I don't want to conflict it in the curated extension json which we will be sending to scaffoldeth.io

name => Something human-readable and useful for displayability on UI.

extensionFlagValue => code name, something which people will pass as -e flag to CLI.

Thanks all! hope it makes sense, but happy to update it either way 🙌

src/curated-extensions.ts Outdated Show resolved Hide resolved
Copy link
Member

@rin-st rin-st left a comment

Choose a reason for hiding this comment

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

Lgtm!

Copy link
Member

@damianmarti damianmarti left a comment

Choose a reason for hiding this comment

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

Perfect! Thanks!!

@damianmarti damianmarti merged commit 4fbbfb4 into main Nov 26, 2024
1 check passed
@damianmarti damianmarti deleted the curated-extension-branch-optional branch November 26, 2024 13:12
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.

3 participants