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

Total refactor #31

Merged
merged 8 commits into from
Mar 10, 2019
Merged

Total refactor #31

merged 8 commits into from
Mar 10, 2019

Conversation

konradhalas
Copy link
Owner

@konradhalas konradhalas commented Feb 4, 2019

This is a total refactor of dacite library.

I split everything into smaller modules, now real unit testing is finally possible. I also changed the way of building data class, current solution is much more generic, e.g. it supports fields like x: List[List[List[Union[int, X]]]]

@jasisz @bpeake-illuscio @rominf - It would be great if anyone of you could find the time to do a short code review.

@konradhalas konradhalas self-assigned this Feb 4, 2019
@konradhalas konradhalas changed the title WIP: Split the main lib module into smaller ones Total refactor Feb 24, 2019
@rominf
Copy link
Contributor

rominf commented Feb 24, 2019

@konradhalas, I'm not sure that I'll be able to do it faster than a week (have a tight schedule this week), but I'm definitely interested in this.

@rominf
Copy link
Contributor

rominf commented Feb 24, 2019

As for now, I don't like the commit messages, they are not descriptive. I think probably that you want to merge them into one if you cannot describe what you did clearly.

@rominf
Copy link
Contributor

rominf commented Feb 24, 2019

Also, consider splitting import into groups: typing and dataclasses should go into the first group, as they come with the standard library.

@konradhalas
Copy link
Owner Author

@rominf thank you, I want to merge it in a few days (probably during weekend) so it will be awesome if you could find some time.

As for now, I don't like the commit messages, they are not descriptive. I think probably that you want to merge them into one if you cannot describe what you did clearly.

I will squash everything into single commit.

Also, consider splitting import into groups: typing and dataclasses should go into the first group, as they come with the standard library.

Which file is wrong? I think that they are grouped as you said. I'm using PyCharm and it is done automatically.

@rominf
Copy link
Contributor

rominf commented Mar 1, 2019

Which file is wrong? I think that they are grouped as you said. I'm using PyCharm and it is done automatically.

In test files you have the following (or something like that):

import pytest
from dataclasses import dataclass, field
from typing import Any

Please change it to:

from dataclasses import dataclass, field
from typing import Any

import pytest

tests/core/test_collection.py Outdated Show resolved Hide resolved
dacite/config.py Show resolved Hide resolved
dacite/dataclasses.py Show resolved Hide resolved
@konradhalas
Copy link
Owner Author

Which file is wrong? I think that they are grouped as you said. I'm using PyCharm and it is done automatically.

In test files you have the following (or something like that):

import pytest
from dataclasses import dataclass, field
from typing import Any

Please change it to:

from dataclasses import dataclass, field
from typing import Any

import pytest

Ha, interesting, this is because I was using Python 3.6 locally and I had dataclass installed as a regular package from PyPI instead of standard lib module. Fixed.

@konradhalas konradhalas merged commit 14b7ac0 into master Mar 10, 2019
@konradhalas konradhalas deleted the split-main-module branch January 4, 2023 10:33
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.

None yet

2 participants