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

skipQuickStart param for README template #144

Merged
merged 3 commits into from
Oct 29, 2024
Merged

Conversation

rin-st
Copy link
Member

@rin-st rin-st commented Oct 23, 2024

Adds possibility to skip default Quick start block of README when creating extensions

Changed a bit newlines so it wont generate 2-3 empty lines depending on different options (well, sometimes it could be empty lines depending on extra lines of parameters or/plus for example when there's no extraContent; but it's still better than it was before, at least with skipQuickStart: true since it adds new line because default quick start block becomes "")

See reason and how to test here

@rin-st rin-st changed the title feat: skipeQuickStart param for README template skipQuickStart param for README template Oct 23, 2024
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.

This is great Rinat! Just a question, I see only testCommand part uses hardcode YourContract.sol above it, everything seems general? Like desc for three magic commands yarn chain, yarn deploy, yarn start seems general right and they should ideally work in all extensions repo?

Just being picky here, also I think this generated README doesn't hold that much value I feel.

For extensions developer their extension's github repo README is important I feel and people using their extension will look at their extension's README and when they public their final product to github they will update this READE any which ways.

So yeah even your sections makes sense!

@rin-st
Copy link
Member Author

rin-st commented Oct 27, 2024

Just a question, I see only testCommand part uses hardcode YourContract.sol above it, everything seems general? Like desc for three magic commands yarn chain, yarn deploy, yarn start seems general right and they should ideally work in all extensions repo?

Yes, except challenge 6, there's also yarn backend-local link .

But despite this block is general, I think for challenges it's important that it should be description -> quick start (checkpoint 0) -> everything else (checkpoints 1+) as it is working now for every sre challenge. If we use the template for generating instance README without this PR changes, we can add (checkpoints 1+) to extra content, so it will be requirements/quick start (instead of checkpoint 0) -> description of the challenge -> everything else (checkpoint 1+) and it feels strange. Because for example you're starting the app and then starting to read it's description

It could also be requirements/quick start part -> full challenge text as extra content, so this PR changes also are not required. In this case yarn chain/deploy/start part will be repeated, so it's not good as well

Just being picky here, also I think this generated README doesn't hold that much value I feel.

For extensions developer their extension's github repo README is important I feel and people using their extension will look at their extension's README and when they public their final product to github they will update this READE any which ways.

If I understood you, you're proposing to generate only default readme for instances, right? And if users want to change README's of their instances, they should do it before publishing to gh? In this case many people will publish their SRE solutions to gh without changing README, and hence most part of all those solutions will have default README with no information about challenge and also with YourContract.sol

@carletex
Copy link
Member

Just being picky here, also I think this generated README doesn't hold that much value I feel.

Yeah, and in the challenges, most people will read it on the SRE site (which is requesting the content from the GitHub README of the repo)

In any case, it makes sense to have a README that makes sense. Not feeling super strong about anything, but the idea of Rinat skipping the quick start section makes sense to me.

This is great Rinat! Just a question, I see only testCommand part uses hardcode YourContract.sol above it, everything seems general? Like desc for three magic commands yarn chain, yarn deploy, yarn start seems general right and they should ideally work in all extensions repo?

Maybe we change the default to:

- Edit your smart contracts in ${contractsPath[0]}

So You either:

  1. use the defaults + maybe add some extra content (which makes sense for most of the extensions)
  2. skip the quick start + add extra content with the stuff you want.

What do you guys think?

@rin-st
Copy link
Member Author

rin-st commented Oct 28, 2024

Maybe we change the default to:
Edit your smart contracts in ${contractsPath[0]}

It would be great, but no contracts deployed when generating readme, so we don't know the final order of contracts.

So You either:

  1. use the defaults + maybe add some extra content (which makes sense for most of the extensions)

Copying this again for this case from #144 (comment)

If we use the template for generating instance README without this PR changes, we can add (checkpoints 1+) to extra content, so it will be requirements/quick start (instead of checkpoint 0) -> description of the challenge -> everything else (checkpoint 1+) and it feels strange. Because for example you're starting the app and then starting to read it's description

So it will be something like this for example for challenge 0

  • requirements (nodejs, yarn etc)
  • yarn chain/deploy/start
  • title, description, youtube links etc after we already started the app
image - Omit checkpoint 0, show 1+ checkpoints

It's not very critical, just tried to make it clearer

So I'm voting for

  1. skip the quick start + add extra content with the stuff you want.

@carletex
Copy link
Member

carletex commented Oct 28, 2024

Hey Rinat, sorry, I didn't explain myself correctly

Maybe we change the default to:
Edit your smart contracts in ${contractsPath[0]}

It would be great, but no contracts deployed when generating readme, so we don't know the final order of contracts.

Shiv mentioned here #144 (review) the issue with YourContract.sol. Everything in that section is generic, except that part. That's why I was suggesting to change:

From

 Edit your smart contract \`YourContract.sol\` in ${contractsPath[0]}

To

 Edit your smart contracts in ${contractsPath[0]}

So YourContract.sol doesn't confuse people (e.g. when you are using the ERC20 extension)


So I'm voting for
skip the quick start + add extra content with the stuff you want.

When I was mentioning these two:

So You either:

  1. use the defaults + maybe add some extra content (which makes sense for most of the extensions)
  2. skip the quick start + add extra content with the stuff you want.

I was referring on how users will use the README template (That the current PR allows!)

  1. Most of the extensions (erc20, etc): defaults (with the fix on YourContract.sol) + maybe the add something else if they want
  2. Remove quick start + add add extra content as the README (e.g. Challenges)

This PR allows both scenarios, right? So I vote for the current approach.

@rin-st
Copy link
Member Author

rin-st commented Oct 28, 2024

So YourContract.sol doesn't confuse people (e.g. when you are using the ERC20 extension)

Now I understand, thank you for clarification, updated!
Yes, agree!

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.

This makes sense! Thanks all!

@technophile-04 technophile-04 merged commit c29c59d into main Oct 29, 2024
1 check passed
@technophile-04 technophile-04 deleted the skip-quick-start branch October 29, 2024 06:38
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