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

mintlist-cli: Adding a bump comand that diffs mintlists and overrides #36

Merged
merged 9 commits into from
Apr 27, 2023

Conversation

terranmoccasin
Copy link
Contributor

@terranmoccasin terranmoccasin commented Mar 28, 2023

Adding a bump command that diffs mintlists to update the package version.

packages/mintlist-cli/src/cmd/bump.ts Outdated Show resolved Hide resolved
// Returns true if a mintlist.json file was added or removed, false otherwise
function hasMintlistChanges(before: string, after: string) {
const beforeMintlists = toMintlistFilePaths(
execSync(`git ls-tree --name-only ${before} src/mintlists`, {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the restriction that mintlist files can only be placed under src/mintlists is present elsewhere in this command.
How about leaving it to the filtering process in toMintlistFilePaths ? (not provide path here)

Copy link
Contributor

Choose a reason for hiding this comment

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

@terranmoccasin
Oh, sorry.

My point is that this bump constrains mintlist to be under src/mintlists directory only, but add-mint/remove-mint/gen-tokenlist does not have that constraint. ./hoge.mintlist.json and ./src/fuga.mintlist.json also appear to be possible.

As with overrides.json, how about forcing the mintlist file to be placed under src/mintlists, or have bump detect the mintlist file for the entire directory.

Personally, I think it is reasonable to force users to use the following directory/file structure.

src/overrides.json
src/mintlists/x.mintlist.json
src/mintlists/y.mintlist.json
...

packages/mintlist-cli/src/cmd/bump.ts Outdated Show resolved Hide resolved
packages/mintlist-cli/src/cmd/bump.ts Outdated Show resolved Hide resolved
packages/mintlist-cli/src/cmd/bump.ts Show resolved Hide resolved
@terranmoccasin
Copy link
Contributor Author

Hey @yugure-orca thanks for the suggestions. I've addressed them all and this is ready for another look!

@yugure-orca
Copy link
Contributor

@terranmoccasin
Thanks for the update!
I additionally commented only on how to find mintlist files in hasMintlistChanges. 🙏
Simple usage by Orca itself should not be a problem, but the flexibility of the options creates a problem.
IMO, it would be less trouble and less inconvenient to constrain it than to free it.

Copy link
Contributor

@yugure-orca yugure-orca left a comment

Choose a reason for hiding this comment

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

LGTM!

@terranmoccasin terranmoccasin merged commit 5333291 into main Apr 27, 2023
@terranmoccasin terranmoccasin deleted the tmoc/mintlist-cli-2 branch April 27, 2023 21:23
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.

2 participants