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

Use cache atomically #7

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lcobucci
Copy link

Just a minor improvement on how the cache component works, allowing it to better encapsulate its concepts.

lcobucci added 2 commits June 27, 2022 20:24
This introduces an alternative method for the cache helper that allows
reducing the public API and removes the need for coordinating method
calls.

It also reduces the amount of method calls needed to use the object,
which is usually irrelevant for small projects but should show
improvements when scanning several files.

Signed-off-by: Luís Cobucci <[email protected]>
This removes unused methods and their tests.

Signed-off-by: Luís Cobucci <[email protected]>
@lcobucci
Copy link
Author

@DaveLiddament is this relevant? I can rebase it but will only invest in it if you see value 😁

@DaveLiddament
Copy link
Owner

@lcobucci thanks for this PR. Sorry I have neglected it.

I'm have a think about the caching in general. I'm wondering when using the reflection provider, I should store the attributes for all the rules and cache those. Then we are only doing one reflection call for each method per analysis run, rather than per rule per analysis run.

I'll get back to you.

Thanks once again for your interest in this project.

@lcobucci
Copy link
Author

It's all good @DaveLiddament, this was just me trying to clean up my list of open PRs 🤣

Take your time and I'm quite alright with having this closed as outcome. No hard feelings there.

Cheers

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