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 generate templates command #1242

Merged
merged 28 commits into from
Oct 30, 2023
Merged

Conversation

chasefleming
Copy link
Member

@chasefleming chasefleming commented Oct 24, 2023

Closes #1233


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@chasefleming chasefleming added the Feature A new user feature or a new package API label Oct 24, 2023
Copy link
Contributor

@gregsantos gregsantos left a comment

Choose a reason for hiding this comment

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

@jribbink can check the go code

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
internal/super/generate.go Outdated Show resolved Hide resolved
internal/super/generate.go Outdated Show resolved Hide resolved
internal/super/generate.go Outdated Show resolved Hide resolved
internal/super/generate.go Outdated Show resolved Hide resolved
internal/super/generate.go Outdated Show resolved Hide resolved
internal/super/generate.go Outdated Show resolved Hide resolved
internal/super/generate.go Outdated Show resolved Hide resolved
internal/super/generate.go Outdated Show resolved Hide resolved
internal/super/generate.go Outdated Show resolved Hide resolved
internal/super/generate.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2023

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (670fac7) 40.30% compared to head (2adf137) 41.19%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1242      +/-   ##
==========================================
+ Coverage   40.30%   41.19%   +0.89%     
==========================================
  Files          38       39       +1     
  Lines        2007     2068      +61     
==========================================
+ Hits          809      852      +43     
- Misses       1109     1123      +14     
- Partials       89       93       +4     
Flag Coverage Δ
unittests 41.19% <70.49%> (+0.89%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
internal/super/generate.go 70.49% <70.49%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jribbink jribbink left a comment

Choose a reason for hiding this comment

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

lgtm

internal/super/generate.go Outdated Show resolved Hide resolved
internal/super/generate_test.go Outdated Show resolved Hide resolved
internal/super/generate_test.go Outdated Show resolved Hide resolved
internal/super/generate_test.go Outdated Show resolved Hide resolved
@chasefleming chasefleming dismissed bjartek’s stale review October 25, 2023 23:45

Addressed feedback


filenameWithBasePath := filepath.Join(basePath, filename)

if _, err := os.Stat(filenameWithBasePath); err == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

flow-cli has the --save -s flag to save the output of a command to a file, do we need to have this logic here or can we just return the output and rely on that logic?

also state has a ReaderWriter that is used to save files and that is used elsewhere instead of os functions.

Copy link
Member Author

@chasefleming chasefleming Oct 26, 2023

Choose a reason for hiding this comment

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

Hmm yeah I guess that means that you always have to provide --save. I was hoping to just always generate them to the correct folders unless specified otherwise. But it makes sense to keep the same pattern. Just really verbose now. Writing flow generate contract woohoo --save cadence/contracts/whoohoo.cdc is so long that I'd almost rather just write the cadence instead of use this command. Makes me want to kill this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Imo just skip the pattern/flag here. I think think it's pretty obvious that this will be saving a file and using a flag for this is just redundant & a command like this is for ease of use so should be as shorthand as possible. I think the only purpose --save potentially has in this is as an override to the output path.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fully in support for super short aliases here as well (i.e. flow g c FooBar)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to not use --save. But using ReaderWriter is recommended. I think it also creates required directories.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah ReaderWriter is no problem. Good catch. Will change that @bjartek

@jribbink
Copy link
Contributor

Also I think it would be cool if flow generate contract added a contract to flow.json automatically. It would make this tool super useful as it saves a bunch of steps making a new contract and significantly speeds up the time scaffolding a new project.

Additionally, there won't be any confusion about imports not being resolved when these contracts are referenced since they are automatically in the flow.json

@bjartek
Copy link
Collaborator

bjartek commented Oct 27, 2023

Totally agree on this. Would it be possible to optionally have it added to deployment section?

One pattern i use in flow.json is that my accounts are named on the form of network-role

So emulator-find, testnet-find, mainnet-find.

@chasefleming
Copy link
Member Author

So here's what I think is a good route based on what you all said. Tell me if you agree @bjartek @jribbink :

  • Switch to ReaderWriter.
  • This feels like a slightly separate pattern and is meant for convenience so I think not using --save to avoid redundancy is fine here since the output is always to create a new file.
  • Let's not use flow g c Foo but I think the alias for generate is fine. I think flow gen contract Foo or flow g contract Foo might be nice.
  • Add the contract to flow.json upon creation.
  • Deployments sounds like a rabbit hole. Like if accounts for each network don't exist are we supposed to create the accounts as well? We already have flow config add deployment which has a nice interactive way of adding deployments to what networks and what accounts. How about we add a message at the end promoting that command and say use it to add deployments.

Thoughts?

@bjartek
Copy link
Collaborator

bjartek commented Oct 27, 2023

That works. It is always podsible to expand later.

@chasefleming
Copy link
Member Author

@jribbink @bjartek I updated the PR to have the "g" alias and to save the contract to flow.json. I'm having some issues with the ReaderWriter. It doesn't actually create the directory on WriteFile so most of the code still needs to be there. And then the tests with it become tricky if I'm trying to check the directory actually have the file created. There are mocks but I'm still figuring out if it's possible to test that with the mocks.

@bjartek
Copy link
Collaborator

bjartek commented Oct 27, 2023

Ammend the code in ReaderWriter then?

@chasefleming
Copy link
Member Author

@bjartek yep working on that next

@chasefleming chasefleming requested a review from bjartek October 30, 2023 19:47
Copy link
Collaborator

@bjartek bjartek left a comment

Choose a reason for hiding this comment

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

LGTM

@chasefleming chasefleming merged commit 800fb88 into master Oct 30, 2023
6 checks passed
@chasefleming chasefleming deleted the chasefleming/generate-templates branch October 30, 2023 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature A new user feature or a new package API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command to create boilerplate
5 participants