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

Add type hints #146

Merged
merged 19 commits into from
May 18, 2024
Merged

Add type hints #146

merged 19 commits into from
May 18, 2024

Conversation

Conengmo
Copy link
Member

@Conengmo Conengmo commented Oct 16, 2023

  • Add type hints to all modules in Branca.
  • Make sure Mypy passes.

Contains some (internally) functional changes as well:

  • Alter _parse_hex to output RGBA floats directly. We only use that function inside of _parse_color, which needs RGBA floats as output.
  • No longer have bytes as accepted type for color strings. That's a remnant from the Python 2 times.
  • Merge duplicate code in Link, JavascriptLink and CssLink.

Do separately later:

  • make subclasses of Figure use the same types for width and height. Also a separate PR I think. Would be easiest to have _parse_size just return a single string.
  • simplify the _camelify function. Is it even needed?
  • remove get_templates

@Conengmo Conengmo marked this pull request as ready for review October 16, 2023 14:11
branca/utilities.py Outdated Show resolved Hide resolved
@Conengmo Conengmo changed the title Add type hints [WIP] Add type hints Apr 4, 2024
@Conengmo
Copy link
Member Author

Conengmo commented May 6, 2024

@ocefpaf would you mind taking a look at this PR at some time? No hurry. I know it's pretty big, but I don't know how to do it otherwise, it's just a lot of type hints. I'm open to suggestions! I'm asking you since you also did python-visualization/folium#1677.

@ocefpaf
Copy link
Member

ocefpaf commented May 7, 2024

@ocefpaf would you mind taking a look at this PR at some time? No hurry. I know it's pretty big, but I don't know how to do it otherwise, it's just a lot of type hints. I'm open to suggestions! I'm asking you since you also did python-visualization/folium#1677.

Sure. I don't have a lot of experience but we can try to run mypy here to check things? I'll see what we can do. There are also some pre-commits that can help.

PS: we need to fix a conflict but I'm looking at it now.

- name: Mypy test
shell: bash -l {0}
run: |
mypy branca
Copy link
Member

Choose a reason for hiding this comment

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

Ha! You are always one (many actually) step ahead of me.

@Conengmo
Copy link
Member Author

Conengmo commented May 7, 2024

Thanks Filipe, much appreciated! I’ll solve the merge conflict before merging. Good idea to add a precommit config as well, I’ll do that in a separate PR.

@Conengmo Conengmo merged commit 8a8a214 into python-visualization:main May 18, 2024
10 checks passed
@Conengmo Conengmo deleted the type-hints branch May 18, 2024 09:00
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