Skip to content

BDE-2483 - Typing change of tags mapping#28

Merged
LynxEko merged 1 commit intomasterfrom
feature/BDE-2483_typing_change_of_tags_mapping
Jun 4, 2025
Merged

BDE-2483 - Typing change of tags mapping#28
LynxEko merged 1 commit intomasterfrom
feature/BDE-2483_typing_change_of_tags_mapping

Conversation

@LynxEko
Copy link
Copy Markdown
Contributor

@LynxEko LynxEko commented Jun 2, 2025

BDE-2483

This PR is part of a series

💡 Context

While reviewing the library to do a talk about pypendency I have found some possible issues.

There is currently a warning in this line:
image

The issue is that we type _tags_mapping as a dict[tag, list(str)] here
image
so there is a mismatch with the defined type and the actual value

typing the _tags_mapping with the proper type solves the warning.

📖 Summary

This PR changes the type definition of _tags_mapping to dict[tag, set(str)]

🧪 How should this be manually tested?

You can manually run the tests with:

docker build . -t pypendency-dev
docker run -v $(pwd)/.:/usr/src/app pypendency-dev bash -c "pipenv run make run-tests"

@LynxEko LynxEko self-assigned this Jun 2, 2025
@LynxEko LynxEko requested a review from a team as a code owner June 2, 2025 09:08
for definition in definitions
}
self._tags_mapping: Dict[Tag, List[str]] = {}
self._tags_mapping: Dict[Tag, Set[str]] = {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we are modifying this, I think that we can also use the built-in types (dict instead of Dict, set instead of Set) to avoid adding an unneeded dependency. WDYT?

Copy link
Copy Markdown
Contributor Author

@LynxEko LynxEko Jun 4, 2025

Choose a reason for hiding this comment

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

I'd love this, however the package still targets python 3.8, and it does not have the built-in types 😢

Copy link
Copy Markdown
Contributor

@andrsdt andrsdt Jun 4, 2025

Choose a reason for hiding this comment

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

Tiny clarification: Built-in types are supported (like set), but they're not subscriptable (like set[str]).
Anyway, as you say, we can't use it here 🙁 we will drop support for 3.8 soon™ 🔥

@eduvives
Copy link
Copy Markdown

eduvives commented Jun 4, 2025

Before:

image

After:

image

Well spotted!
✅ The warning no longer appears on the corresponding line.

Tests still Pass:

Screen.Recording.2025-06-04.at.11.31.24.mov

Copy link
Copy Markdown
Contributor

@andrsdt andrsdt left a comment

Choose a reason for hiding this comment

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

LGTM ✅

Great job guys! 🔝🥇

@LynxEko LynxEko merged commit 70ca0db into master Jun 4, 2025
3 checks passed
@jorgemo-fever jorgemo-fever deleted the feature/BDE-2483_typing_change_of_tags_mapping branch June 4, 2025 15:25
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.

4 participants