-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: BACK-8055 Detect unused definitions #154
base: main
Are you sure you want to change the base?
Conversation
90a55e1
to
32c7e3f
Compare
32c7e3f
to
c41d245
Compare
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
36fa848
to
d418e6d
Compare
2dcc97e
to
b6b597d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes no sense to implement this at ERC7730Linter
level, since it works on resolved descriptors where we already have inlined references.
I think, the right place is to do it ERC7730InputToResolved
, since this is where we use references. Just keep track of which references are used at least once in ERC7730InputToResolved
, for instance by using a wrapping container that counts calls, and output a warning at the end if some have never been called.
@@ -13,5 +14,5 @@ class ERC7730Linter(ABC): | |||
""" | |||
|
|||
@abstractmethod | |||
def lint(self, descriptor: ResolvedERC7730Descriptor, out: OutputAdder) -> None: | |||
def lint(self, descriptor: ResolvedERC7730Descriptor | InputERC7730Descriptor, out: OutputAdder) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can't be changed like this, linter runs on resolved descriptors. This only tricks the (python) linter but at runtime it takes resolved descriptors (see main class)
for future in (executor.submit(lint_file, file, input_linter, out, label(file)) for file in files): | ||
future.result() | ||
|
||
with ThreadPoolExecutor() as executor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why run twice ?
resolved_descriptor = ERC7730InputToResolved().convert(input_descriptor, out) | ||
if resolved_descriptor is not None: | ||
linter.lint(resolved_descriptor, out) | ||
if isinstance(linter, DefinitionLinter): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can see here that it doesn't fit. Interface is not good if you have to do special cases here
This pr aims to enrich linter with unused field definition capability. If a field exists under descriptor.display.definition, it should be used elsewhere under format.
JIRA Ticket