refactor[types]: remove unused _NodeMetadataJournal - #5221
Conversation
Gas ChangesNo changes detected. Summary
|
📊 Bytecode Size Changes (venom)No changes detected. Full bytecode sizes
|
charles-cooper
left a comment
charles-cooper
left a comment
There was a problem hiding this comment.
hang on, it is used though? like we are using NodeMetadata. the journal functionality is unused, but that doesn't really bother me..
|
@charles-cooper There is also be a performance impact to having that journal around, but I have not tested to see if it is significant Feel free to close as you see fit |
oh yea, i'm not concerned about the performance impact. and yes we do not need the journal anymore.. we could keep NodeMetadata as a class but delete the journal part |
|
@charles-cooper |
type safety / hygiene / better signatures |
|
relatedly makes it easier to refactor if we ever want to add or change functionality to the node metadata |
|
Done: f43f083 But as you can see, I'm not sure this adds much |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f43f0839f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,83 +1,11 @@ | |||
| import contextlib | |||
There was a problem hiding this comment.
With the journal code removed, this file is no longer formatted the way CI expects: the workflow runs black --check -C --force-exclude=vyper/version.py ./vyper ./tests ./setup.py and isort --check-only --diff ./vyper ./tests ./setup.py, and both checks now report that vyper/ast/metadata.py needs an extra blank line after this import (black also removes the trailing blank line at EOF). Please run the formatter or remove the now-unused import so the lint job stays green.
Useful? React with 👍 / 👎.
charles-cooper
changed the title
_NodeMetadataJournal
|
@Sporarum approved, please fix lint and i think we should also add a |
I had forgotten to push it ...
I agree ! However, I don't think it applies for this PR, the issue being neither does any others :/ |
charles-cooper
changed the title
_NodeMetadataJournal_NodeMetadataJournal
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
2 participants
What I did
_NodeMetadataJournal was unused since ddfce52 (from that commit message):
It is now more than 2 years later, and it hasn't been used since.
(and this commit can always be reverted if that need comes)
How I did it
Remove
_NodeMetadataJournal,NodeMetadata(became a no-op dict wrapper), and associated tests.How to verify it
pytestCommit message
Description for the changelog
(refactor, no user-visible change)
Cute Animal Picture