Moved metadata_store into database and content_discovery #7773
Conversation
qstokkink
force-pushed
the
upd_db_class_locations
branch
6 times, most recently
from
47ca1d1
to
d582312
Compare
qstokkink
force-pushed
the
upd_db_class_locations
branch
from
d582312
to
39d5db7
Compare
qstokkink
changed the title
| db_path = ":memory:" | ||
|
|
||
| self.db = TriblerDatabase(str(db_path)) | ||
|
|
||
| # TODO: merge the code below into the TriblerDatabase |
There was a problem hiding this comment.
Is this a TODO for the next PR, or is it a forgotten TODO for the current PR?
There was a problem hiding this comment.
This is for the next PR.
There was a problem hiding this comment.
Some of the developers and I in this group consider it bad, or let's say "not good," practice to merge changes with TODOs into the main branch.
I'm not stopping you from doing this by not giving acceptance to the PR. I'm just highlighting that there are many points of view on this practice, some of which consider it to be a bad practice that leads to code quality degradation.
There was a problem hiding this comment.
I agree with them. However, our current linters are configured to allow this. Notably, I did not have to disable any warnings to pass our PR checks.
Once we fix #1722 we can adopt a more strict stance on this.
There was a problem hiding this comment.
Are you saying that if you understand there is a bad practice, but there are no configured linter issues, then it is okay for you to commit and merge the code despite its quality?
There was a problem hiding this comment.
Even though I disagree with the practice personally, this is the approach that the team has chosen and I adapt my workflow to it.
If I were the only developer, I would make make own standards and conform to what I would like to see. Sadly, I am not the only developer and I have to conform to the existing rules and linter settings.
qstokkink
changed the title
Related to #7669, following the plan of #7669 (comment)
The changes of this PR are simply moves and dead code removal.
Unit tests have been added to cover missing lines (in our existing code). The lines in
database_component.pythat are part ofgui_test_modeorchant_testnetare not covered.