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

Improve DB handling #451

Merged
merged 3 commits into from
Oct 26, 2023
Merged

Improve DB handling #451

merged 3 commits into from
Oct 26, 2023

Conversation

ferdinandjarisch
Copy link
Contributor

No description provided.

This commit changes the DB behavior such that each invocation of gallia
BaseCommands can be logged to the DB with their respective META if the
--db flag is provided.

Breaking because the config location for db changed fromm
[gallia.scanner] to plain [gallia].
@ferdinandjarisch ferdinandjarisch removed the request for review from peckto October 10, 2023 12:26
@rumpelsepp
Copy link
Member

The DB is meant to be a Scanner feature; it provides very specific UDS features. Moving this into base is not a good fit.

@ferdinandjarisch
Copy link
Contributor Author

it provides very specific UDS features

Those very specific UDS features are not utilized here.
Only the very generic DB features are utilized, that add generic META infos to the DB are.

Why doesn't it make sense to have this in the DB as well?

@rumpelsepp
Copy link
Member

Why doesn't it make sense to have this in the DB as well?

We do not have a generic database class yet. From a datatype perspective, this mixes specific code with generic code and in my opinion this is a bad idea. You might not use the specific code in your patch, but the code is there. From my experience this will eventually grow and the code will end up more difficult to maintain.

So if you really want to have this, we need to separate the database code.


During writing this I looked up the code and I realized that what I am saying is actually not true. 👯 🐒 The DB is part of Scanner and not UDSScanner. So, this in any case needs to be cleaned up for my argument to be valid. I think you could move it and we clean up later (tm). Opinions?

@ferdinandjarisch ferdinandjarisch merged commit 603cef0 into master Oct 26, 2023
@ferdinandjarisch ferdinandjarisch deleted the improve-db branch October 26, 2023 08:41
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