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

Extension to solidity framework #68

Merged
merged 14 commits into from
Jul 5, 2024

Conversation

rin-st
Copy link
Member

@rin-st rin-st commented Jun 25, 2024

  • rename extensions to solidityFramework (option, folder, vars, types, docs etc). solidityFramework option became a single value instead of extensions array
  • removed config.json logic and readme since as I understand it's not used at all

Fixes #67

@technophile-04
Copy link
Collaborator

Hey Rinat could just fix the merge conflicts I think they were caused due to #55

@rin-st
Copy link
Member Author

rin-st commented Jun 29, 2024

Thanks! Fixed

src/tasks/copy-template-files.ts Outdated Show resolved Hide resolved
src/tasks/copy-template-files.ts Outdated Show resolved Hide resolved
src/tasks/copy-template-files.ts Outdated Show resolved Hide resolved
src/utils/consts.ts Outdated Show resolved Hide resolved
src/tasks/copy-template-files.ts Outdated Show resolved Hide resolved
src/utils/extensions-dictionary.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

Thanks @rin-st !! This is really nice!!

I was also since we are now clear we are no more having any extensions in repo, maybe remove :

extensions: [SOLIDITY_FRAMEWORKS.HARDHAT, SOLIDITY_FRAMEWORKS.FOUNDRY, null],

I think we can also remove this logic :

config.questions.forEach(question => {
if (invalidQuestionNames.includes(question.name)) {
throw new Error(
`The name of the question can't be "${question.name}". The invalid names are: ${invalidQuestionNames
.map(w => `"${w}"`)
.join(", ")}`,
);
}
const extensions = question.extensions.filter(isDefined);
const hasNoneOption = question.extensions.includes(null);
questions.push({
type: question.type === "multi-select" ? "checkbox" : "list",
name: question.name,
message: question.message,
choices: hasNoneOption ? [...extensions, nullExtensionChoice] : extensions,
});
});

and simplify this logic too, since things are now deterministic :

create-eth/src/types.ts

Lines 42 to 62 in 170bc6d

interface SolidityFrameworkQuestion<T extends SolidityFrameworkOrNull[] = SolidityFrameworkOrNull[]> {
type: QuestionType;
extensions: T;
name: string;
message: Question["message"];
default?: T[number];
}
/**
* This function makes sure that the `T` generic type is narrowed down to
* whatever `extensions` are passed in the question prop. That way we can type
* check the `default` prop is not using any valid extension, but only one
* already provided in the `extensions` prop.
*
* Questions can be created without this function, just using a normal object,
* but `default` type will be any valid Extension.
*/
export const typedQuestion = <T extends SolidityFrameworkOrNull[]>(question: SolidityFrameworkQuestion<T>) => question;
export type Config = {
questions: SolidityFrameworkQuestion[];
};

@rin-st
Copy link
Member Author

rin-st commented Jul 3, 2024

Thank you for thorough review Shiv!

Regarding your last message

I was also since we are now clear we are no more having any extensions in repo, maybe remove :

I think we can also remove this logic :

and simplify this logic too, since things are now deterministic :

I removed some types, simplified prompt logic and changed config.ts to curated-extensions.ts since there's nothing more in that file except curated extensions 4fd327b

Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

Really awesome work Rinat!! Tested everything throughly out (solidity frameworks, external extension, curated extension, + dev mode) and everything works nicely!!

Just last small thing could update this too? :

I propose we treat anything other than "base" as an extension. That way we don't need to make distinctions between solidity frameworks or any other kind of extension.
This change should make it easier to grow the options we provide our users without having to classify extensions by category.
This change requires a new file, `src/config.ts`, where a few things about the extensions are defined, e.g. the sequence of questions about extensions.
# Config files
There's one main `src/config.ts` file to configure the questions shown to the user.
| ⚠️ Note how the extension config file is a JSON file
## Config files API
### `src/config.ts`
Have a look at `src/types.ts#Config` or the file itself.
Just a quick note to mention that adding `null` as an item in the `extensions` property will show the "None" option to the user. That option doesn't add any extension for the given question.

@rin-st
Copy link
Member Author

rin-st commented Jul 4, 2024

Just last small thing could update this too? :

I propose we treat anything other than "base" as an extension. That way we don't need to make distinctions between solidity frameworks or any other kind of extension.
This change should make it easier to grow the options we provide our users without having to classify extensions by category.
This change requires a new file, `src/config.ts`, where a few things about the extensions are defined, e.g. the sequence of questions about extensions.
# Config files
There's one main `src/config.ts` file to configure the questions shown to the user.
| ⚠️ Note how the extension config file is a JSON file
## Config files API
### `src/config.ts`
Have a look at `src/types.ts#Config` or the file itself.
Just a quick note to mention that adding `null` as an item in the `extensions` property will show the "None" option to the user. That option doesn't add any extension for the given question.

Good eye 👍!

2d9c67e Removed the whole text since we don't have extensions other than solidity framework and config is also removed.

Just a quick note to mention that adding null as an item in the extensions property will show the "None" option to the user. That option doesn't add any extension for the given question.

Thought about this part but since we have it only in one place (not even exact same way)

choices: [SOLIDITY_FRAMEWORKS.HARDHAT, SOLIDITY_FRAMEWORKS.FOUNDRY, nullExtensionChoice],
and it's clear how it works so I think we don't need this text too


Renamed core-extensions.md to TEMPLATING.md since file became mostly about templating

@technophile-04
Copy link
Collaborator

and it's clear how it works so I think we don't need this text too
Renamed core-extensions.md to TEMPLATING.md since file became mostly about templating

Yup yup 100% agree!

Thanks @rin-st!!! Merging this!

@technophile-04 technophile-04 merged commit 0a54e5e into main Jul 5, 2024
1 check passed
@technophile-04 technophile-04 deleted the extension-to-solidity-framework branch July 5, 2024 00:31
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.

Rename extentions to solidity framework
2 participants