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

Refactor processing, get rid of strategies.py #98

Merged
merged 5 commits into from
Dec 7, 2021
Merged

Conversation

kissgyorgy
Copy link
Contributor

We decided that we don't want to implement the strategy abstraction right now.
Moved search chunk related functionality into finder.py and deleted strategies.py and more small refacts.

Added unit tests and bumped test coverage requirement.

@kissgyorgy kissgyorgy requested a review from vlaci December 7, 2021 08:59
Copy link
Contributor

@vlaci vlaci left a comment

Choose a reason for hiding this comment

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

I have left a few general comments on the code to bring up points where potential further refactoring is due

unblob/finder.py Show resolved Hide resolved
unblob/finder.py Show resolved Hide resolved
unblob/finder.py Outdated Show resolved Hide resolved
We decided that we don't want to implement the strategy abstraction right now.
Moved search chunk related functionality into finder.py and deleted strategies.py
The functions are ordered in the call order.
Renamed the handler list to "ALL_HANDLERS_BY_PRIORITY" and made it public,
as we import and use them multiple places. This is the official place to
register new handlers.

Load handlers lazily and cache the instantiation.
It's more natural to show those in decimal.
Wrote new unit tests during refactoring, coverage can be increased.
Put the configuration in pyproject.toml, so everyone can run it with
the same configuration.
Reason should be a better phrase.
@kissgyorgy kissgyorgy merged commit 90057a4 into main Dec 7, 2021
@kissgyorgy kissgyorgy deleted the refact-processing branch December 7, 2021 18:28
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