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

Duplicate Entry Handling (Fixes #179) #204

Merged
merged 8 commits into from
May 29, 2024
Merged

Duplicate Entry Handling (Fixes #179) #204

merged 8 commits into from
May 29, 2024

Conversation

CyanVoxel
Copy link
Member

Initially this was intended to be a quick patch that ended up needing a bigger fix, especially when it came to conflicts with the workflows that I was unable to reproduce on my machine. Either way, I should really be making my changes via PRs in the first place...

This PR makes the following changes:

  • Running "Fix Unlinked Entries" will no longer result in duplicate entries if the directory was refreshed after the original entries became unlinked.
  • A "Duplicate Entries" section is added to the "Fix Unlinked Entries" modal to help repair existing affected libraries.
    • Having duplicate entries like this is not something that's supposed to happen in the first place, so this section here is not a long-term solution.

There were a couple bodges in this involving Path casting and applying # type: ignore to problematic lines that set off MyPy, false positives or otherwise.

@CyanVoxel CyanVoxel added Type: Bug Something isn't working as intended Type: UI/UX User interface and/or user experience TagStudio: Library Relating to the TagStudio library system labels May 21, 2024
@CyanVoxel CyanVoxel added this to the Alpha 9.2.1 milestone May 21, 2024
@@ -86,20 +86,20 @@ def __repr__(self) -> str:
return self.__str__()

def __eq__(self, __value: object) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

At this point there's no type check in this method. I'm not sure how how is that better than casting the variable, but if you want disable the type checking then you can add this decorator to the method declaration rather than adding ignore on each line with error.

Suggested change
def __eq__(self, __value: object) -> bool:
@typing.no_type_check
def __eq__(self, __value: object) -> bool:

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the info on the @typing.no_type_check decorator, I'll definitely use that if needed going forward 👍

The casting itself was causing me the following error when invoking the __eq__ method:

Traceback (most recent call last):
  File "F:\GitHub\TagStudio/tagstudio\src\qt\helpers\custom_runnable.py", line 18, in run
    self.function()
  File "F:\GitHub\TagStudio/tagstudio\src\qt\modals\merge_dupe_entries.py", line 44, in <lambda>
    r = CustomRunnable(lambda: iterator.run())
                               ^^^^^^^^^^^^^^
  File "F:\GitHub\TagStudio/tagstudio\src\qt\helpers\function_iterator.py", line 20, in run
    for i in self.iterable():
  File "F:\GitHub\TagStudio/tagstudio\src\core\library.py", line 1043, in merge_dupe_entries
    self.entries.remove(id_to_entry_map[id])
  File "F:\GitHub\TagStudio/tagstudio\src\core\library.py", line 92, in __eq__
    int(self.id) == int(__value.id)
                        ^^^^^^^^^^
AttributeError: type object 'object' has no attribute 'id'

I can reproduce the error by comparing any two Entry objects:

entry_1 = Entry(1, "file_2", "path_2", [])
        entry_2 = Entry(2, "file_2", "path_2", [])
        logging.info(entry_1 == entry_2)

It looks like the object cast is just stripping the Entry's attributes down and turning it back into a bare object, right? I'm not sure how the cast is intended to function here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

right, my bad. It should be this instead:

__value = cast(Self, __value)

@CyanVoxel CyanVoxel modified the milestones: Alpha 9.2.1, Alpha 9.3.x May 23, 2024
@@ -86,20 +86,20 @@ def __repr__(self) -> str:
return self.__str__()

def __eq__(self, __value: object) -> bool:
__value = cast(Self, object)
# __value = cast(Self, object)
Copy link
Collaborator

@yedpodtrzitko yedpodtrzitko May 23, 2024

Choose a reason for hiding this comment

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

Suggested change
# __value = cast(Self, object)
__value = cast(Self, __value)

then the type: ignore can begone

CyanVoxel and others added 3 commits May 23, 2024 13:01
- Remove `type: ignore` comments from `Entry`'s `__eq__` method
- Change the cast in this method from `__value = cast(Self, object)` to `__value = cast(Self, __value)`

Co-Authored-By: Jiri <[email protected]>
@CyanVoxel CyanVoxel merged commit 868b553 into main May 29, 2024
6 checks passed
@CyanVoxel CyanVoxel deleted the dupe-entry-handling branch May 31, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TagStudio: Library Relating to the TagStudio library system Type: Bug Something isn't working as intended Type: UI/UX User interface and/or user experience
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants