Skip to content

Conversation

@avlonder
Copy link
Contributor

No description provided.

@avlonder
Copy link
Contributor Author

On my side, the last benchmark is highlighted in red. Is this a problem?

@mciszczon
Copy link
Collaborator

mciszczon commented Jan 14, 2025

On my side, the last benchmark is highlighted in red. Is this a problem?

Sometimes we might want to trade a small performance hit for highly enhanced functionality, right? But some of the tests still fail, some due to the usage of newer Python functionalities in Python 3.7 and 3.8 environments, some seem to be legit issues with the code.

You can try to run tests on different Python versions locally, and I'll try to also look into it in the upcoming days. Sorry for that, a bit busy right now. But definitely let's push this forward so we can finally merge it into the repo.

@konradhalas What do you think about the change_keys part? I do agree it's a bit out of scope of the project.

@avlonder
Copy link
Contributor Author

avlonder commented Jan 18, 2025

Sometimes we might want to trade a small performance hit for highly enhanced functionality, right? But some of the tests still fail, some due to the usage of newer Python functionalities in Python 3.7 and 3.8 environments, some seem to be legit issues with the code.

Agreed. Especially since the extra compute caused by my changes only needs to be done once as it is wrapped by lru_cache.

@konradhalas What do you think about the change_keys part? I do agree it's a bit out of scope of the project.

For my personal project, I could not use dacite without this feature: my UI expects to receive camelCase keys. Without this feature, you have to loop over the dict twice, which is a bummer wrt performance. Note that convert_keys is a config parameter, so it's optional, and it is not a change that makes the dacite code more difficult to maintain as it hooks in on basically one line of code in the core function.

@avlonder
Copy link
Contributor Author

avlonder commented Jan 22, 2025

Tested locally for

  • 3.8.18
  • 3.9.13
  • 3.10.13
  • 3.11.7
  • 3.12.1

Only the last benchmark is still red.

@mciszczon
Copy link
Collaborator

@avlonder Thanks for the work, we're almost there. There's just two small things. Please take a look at the PR here: #271 and see my two latest commits.

  1. We can add # type: ignore in two places to silence mypy complaining about importing a few modules from typing. These imports will work, but mypy would require manually checking for Python versions in order to read it properly.
  2. In generics.py I extracted the common GenericsDict = Dict[TypeVar, Type] type, but it's problematic in line 52.

Once we have these two fixed we can finally merge and make a new release! 🎉

@avlonder
Copy link
Contributor Author

avlonder commented Jan 24, 2025

  1. We can add # type: ignore in two places to silence mypy complaining about importing a few modules from typing. These imports will work, but mypy would require manually checking for Python versions in order to read it properly.
  2. In generics.py I extracted the common GenericsDict = Dict[TypeVar, Type] type, but it's problematic in line 52.

Ok, looked at it and applied the typing fixes. This generics.get(hint, hint) was a safety measure to fall back to the original TypeVar, but this fallback is AFAIK never hit in my project, so I just discarded it, making the code pass the type checker: if the TypeVar does not occur in the generics dict for a case I was overlooking, the original dacite would have failed anyways, so it actually made no sense to introduce the fallback.

@avlonder
Copy link
Contributor Author

Oh wait, no: the original dacite would have worked in case config.check_types = False. Let me revert that change...

@avlonder
Copy link
Contributor Author

Ready :)

@mciszczon mciszczon mentioned this pull request Jan 27, 2025
@mciszczon mciszczon changed the base branch from master to feature/generics January 27, 2025 15:10
@mciszczon mciszczon merged commit 4d7295a into konradhalas:feature/generics Jan 27, 2025
0 of 7 checks passed
@mciszczon
Copy link
Collaborator

Thanks a tonne @avlonder, I've merged your PR into a separate branch in order to avoid more back-and-forts, will look into why mypy is complaining all the time and will soon sketch out a new version release!

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