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.

Sign up for GitHub

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

Jump to bottom

We need to go through all the TODO/FIXME entries and clean them up/do something about them #1722

Closed
whirm opened this issue Nov 12, 2015 · 5 comments · Fixed by #8057
Closed

We need to go through all the TODO/FIXME entries and clean them up/do something about them #1722

whirm opened this issue Nov 12, 2015 · 5 comments · Fixed by #8057
Assignees
@drew2a
Labels
type: enhancement

Comments

@whirm
Copy link

whirm commented Nov 12, 2015

No description provided.

@whirm whirm modified the milestone: Backlog Mar 18, 2016
@ichorid ichorid closed this as completed Jul 17, 2020
@qstokkink
Copy link
Contributor

Why was this closed? We still have a boatload of TODO's and FIXME's in the code.

@ichorid
Copy link
Contributor

ichorid commented Jul 17, 2020

Because this is useless. We need a separate issue per FIXME/TODO

@qstokkink
Copy link
Contributor

We need a separate issue per FIXME/TODO

I agree, but we don't have this breakdown (yet). Until we do, this issue is not resolved.

@synctext
Copy link
Member

ToDo about our ToDo list..

@synctext synctext reopened this Jul 18, 2020
@qstokkink qstokkink mentioned this issue Dec 14, 2023
Merged
@qstokkink
Copy link
Contributor

qstokkink commented Jan 12, 2024 •
edited
Loading

I think we now have sufficient team support to finally resolve this issue and stop the practice of communication through TODOs and switch fully to GitHub issues for communication between developers. When this is resolved, we should enable the pylint check (W0511) to enforce this.

I created an overview of all FIXME-type issues and what I believe to be the correct response to each of them.

************* Module tribler.gui.utilities
src/tribler/gui/utilities.py:393: [W0511(fixme), ] FIXME: this is a temp fix to cap the normalized value to 1.

Recommended action: Remove this comment without creating an issue.
Rationale: This is simply defensive programming and doesn't need to be fixed. The intent of the code is obvious.

************* Module tribler.gui.widgets.tablecontentdelegate
src/tribler/gui/widgets/tablecontentdelegate.py:143: [W0511(fixme), ] TODO: restore this behavior, so there is really some tolerance zone!
src/tribler/gui/widgets/tablecontentdelegate.py:539: [W0511(fixme), ] TODO: refactor this not to rely on inheritance order, but instead use interface method pattern

Recommended action (143): Remove this comment without creating an issue.
Rationale (143): This is a "nice to have" feature request that is likely to be superseded by #7736
Recommended action (143): Remove this comment without creating an issue.
Rationale (143): This is a good pointer for refactoring but also likely to be superseded by #7736

************* Module tribler.gui.widgets.settingspage
src/tribler/gui/widgets/settingspage.py:494: [W0511(fixme), ] TODO: do it in RESTful style, on the REST return JSON instead

Recommended action: Remove this comment without creating an issue.
Rationale: This comment seems to be out of date.

************* Module tribler.core.components.libtorrent.download_manager.download_manager
src/tribler/core/components/libtorrent/download_manager/download_manager.py:95: [W0511(fixme), ] TODO: Remove the dependency on notifier and refactor it to instead use callbacks injection
src/tribler/core/components/libtorrent/download_manager/download_manager.py:844: [W0511(fixme), ] TODO: refactor this method. It is too long and tightly coupled with higher-level modules.

Recommended action (95): Remove this comment without creating an issue.
Rationale (95): The Notifier has already been removed, where sensible, by #6206
Recommended action (844): Remove this comment without creating an issue.
Rationale (844): I don't agree with the suggested action. The function is only 22 LOC. The coupling seems acceptable.

************* Module tribler.core.components.libtorrent.restapi.torrentinfo_endpoint
src/tribler/core/components/libtorrent/restapi/torrentinfo_endpoint.py:133: # TODO(Martijn): store the stuff in a database!!!
src/tribler/core/components/libtorrent/restapi/torrentinfo_endpoint.py:134: # TODO(Vadim): this means cache the downloaded torrent in a binary storage, like LevelDB
src/tribler/core/components/libtorrent/restapi/torrentinfo_endpoint.py:144: [W0511(fixme), ] FIXME: json.dumps garbles binary data that is used by the 'pieces' field

Recommended action (133,134): Remove this comment without creating an issue.
Rationale (133,134): An issue has already been opened for this at #5669
Recommended action (144): Keep the comment but remove the FIXME tag.
Rationale (144): This is a good note on the behavior of JSON but not something we can fix.

************* Module tribler.core.components.database.database_component
src/tribler/core/components/database/database_component.py:14: [W0511(fixme), ] TODO: legacy, should be merged into ``db``
src/tribler/core/components/database/database_component.py:26: [W0511(fixme), ] TODO: merge the code below into the TriblerDatabase

Recommended action (14,26): Remove this comment once the "blocker" issue is resolved.
Rationale (14,26): An issue has already been opened for this at #7669

************* Module tribler.core.components.database.db.serialization
src/tribler/core/components/database/db/serialization.py:13: [W0511(fixme), ] TODO: move to IPv8

Recommended action: Remove the entire line, including the comment.
Rationale: This format is already part of the default IPv8 formatter, see https://github.com/Tribler/py-ipv8/blob/master/ipv8/messaging/serialization.py#L404

************* Module tribler.core.components.database.category_filter.family_filter
src/tribler/core/components/database/category_filter/family_filter.py:113: [W0511(fixme), ] XXX filter should be stateless

Recommended action: Change the comment to not start with XXX.
Rationale: When starting a comment with XXX, it is denoted as a FIXME-type comment. This causes false-positives (case and point this warning).

************* Module tribler.core.components.database.restapi.database_endpoint
src/tribler/core/components/database/restapi/database_endpoint.py:390: [W0511(fixme), ] TODO: add XXX filtering for completion terms

Recommended action: Create an issue for this.
Rationale: Our XXX filtering is now only half-working. This should indeed be addressed.

This was referenced Jun 5, 2024
Closed
Merged
@drew2a drew2a self-assigned this Jun 5, 2024
@drew2a drew2a modified the milestones: Backlog, 7.15.0 Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@drew2a drew2a

Labels
type: enhancement
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove TODO FIXME and XXX comments drew2a/tribler
5 participants
@synctext @whirm @ichorid @qstokkink @drew2a