Skip to content

✨ feat(cli): et clean #980

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

Merged
merged 13 commits into from
Dec 4, 2024
Merged

✨ feat(cli): et clean #980

merged 13 commits into from
Dec 4, 2024

Conversation

raxhvl
Copy link
Member

@raxhvl raxhvl commented Nov 29, 2024

πŸ—’οΈ Description

Introduces uv run et clean.

πŸ”— Related Issues

closes #976.

βœ… Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

@raxhvl raxhvl changed the title ✨ feat: et clean ✨ feat(cli): et clean Nov 29, 2024
@danceratopz danceratopz added type:feat type: Feature scope:eest Scope: Changes to eest CLI command labels Nov 29, 2024
@danceratopz
Copy link
Member

One thing I do delete every once in a while is the __pycache__ folders.

find src/ -name '__pycache__' -exec rm -rf '{}' \;

We could use glob for this.

@raxhvl, @spencer-tb what do you think?

@spencer-tb
Copy link
Contributor

One thing I do delete every once in a while is the __pycache__ folders.

find src/ -name '__pycache__' -exec rm -rf '{}' \;

We could use glob for this.

@raxhvl, @spencer-tb what do you think?

Definitely agree! Starting fresh without having to reclone for me would the ideal scenario for et clean

I think we should delete cached_downloads/ - the fixtures that are downloaded from the consume command when the input is a url. We could also delete .pyspelling_en.dict.

I'm happy with --all deleting .venv too!

LGTM other than that! Thanks for implementing this @raxhvl - I've wanted this feature for while

@raxhvl
Copy link
Member Author

raxhvl commented Nov 29, 2024

Would we also need a --dry-run command to inspect whats happening?

@danceratopz
Copy link
Member

Would we also need a --dry-run command to inspect whats happening?

πŸ‘ This would be wonderful tbh.

@danceratopz
Copy link
Member

I think we should delete cached_downloads/ - the fixtures that are downloaded from the consume command when the input is a url.

Up for this, but I'd add it to --all :)

@raxhvl
Copy link
Member Author

raxhvl commented Nov 29, 2024

How does this look for default vs --all?

  items_to_remove = [".pytest_cache", ".mypy_cache", "fixtures", "build", "site", "cached_downloads", ".pyspelling_en.dict"]

    if all:
         # Delete __pycache__ directories using glob
         pycache_dirs = glob.glob("**/__pycache__", recursive=True)
         items_to_remove.extend(pycache_dirs)
         
         # Environment
         items_to_remove.extend([".tox", ".venv"])

@danceratopz
Copy link
Member

How does this look for default vs --all?

  items_to_remove = [".pytest_cache", ".mypy_cache", "fixtures", "build", "site", "cached_downloads", ".pyspelling_en.dict"]

    if all:
         # Delete __pycache__ directories using glob
         pycache_dirs = glob.glob("**/__pycache__", recursive=True)
         items_to_remove.extend(pycache_dirs)
         
         # Environment
         items_to_remove.extend([".tox", ".venv"])

Looks good, only suggestion: I would prefer to delete pycache_dirs without all.

I hope I don't need to use --all very often, but will be happy it exists πŸ™‚

@danceratopz
Copy link
Member

@raxhvl @spencer-tb one more location that could be helpful to add to all: The directory containing the cached ethereum/execution-specs forks from the ethereum-spec-evm-resolver, cf this value:
https://github.com/petertdavies/ethereum-spec-evm-resolver/blob/c68756230a27709e10426d5fbe4fa1b142a0b0ee/src/ethereum_spec_evm_resolver/main.py#L43-L45

@raxhvl
Copy link
Member Author

raxhvl commented Nov 29, 2024

The directory containing the cached ethereum/execution-specs forks from the ethereum-spec-evm-resolver.

Is it this dir Path(platformdirs.user_cache_dir("ethereum-spec-evm-resolver"))? I guess we will have to add platformdirs as a dependency.

@raxhvl raxhvl force-pushed the feat/et-clean branch 2 times, most recently from 31d5443 to 495d266 Compare November 29, 2024 11:42
Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

Really nice! Thanks for adding this @raxhvl! Few minor comments below.

@raxhvl raxhvl requested a review from danceratopz December 2, 2024 16:26
Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

Looks good to me! As discussed, feel free to refactor the click CLI docs to use #985.

@raxhvl
Copy link
Member Author

raxhvl commented Dec 3, 2024

Looks good to me! As discussed, feel free to refactor the click CLI docs to use #985.

Done!

Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

Thanks @raxhvl! Nearly there πŸ™‚ two minor comments below.

raxhvl and others added 2 commits December 4, 2024 11:26
Co-authored-by: danceratopz <[email protected]>
Co-authored-by: danceratopz <[email protected]>
@raxhvl
Copy link
Member Author

raxhvl commented Dec 4, 2024

Here is the updated help (truncated):

file for Ethereum execution specifications. The user is prompted to select
  the type of test, the fork to use, and to provide the EIP number and name.
  Based on the inputs, a test file is created in the appropriate directory
  with a rendered template.

  Prompts:

  * Choose the type of test to generate (State or Blockchain)

  * Select the fork where this functionality was introduced

  * Enter the EIP number

  * Enter the EIP name

  Example:

      uv run et make test

Options:
  -h, --help  Show this message and exit.

  Further help: https://ethereum.github.io/execution-spec-tests/main/writing_tests/

Docs:

image

Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this @raxhvl. This will very useful to regular and new contributors alike!

@danceratopz danceratopz merged commit 3aa830a into ethereum:main Dec 4, 2024
3 checks passed
@raxhvl raxhvl deleted the feat/et-clean branch December 11, 2024 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:eest Scope: Changes to eest CLI command type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ feat(et): add eest clean to remove all generated folders and files
3 participants