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

Bugfix duplicate dataclass attributes #100

Merged

Conversation

bstadlbauer
Copy link
Contributor

This should fix the issue of dataclasses having duplicate attributes when these have defaults. This occurs because attributes with defaults are interpreted differently by Python (as they have a default, they already "exist" in the "regular" class).

The solution implemented here first checks if the argument is already present in self.arguments (comparison by name). If it is, it replaces the old argument (as the new one has the dataclass_field property set).

@bstadlbauer
Copy link
Contributor Author

bstadlbauer commented Apr 2, 2021

@pawamoy Note that poetry environment setup is currently not possible due to a bug: python-poetry/poetry#534

My fix locally was to pin pytest-randomly = "3.5.0"

src/pytkdocs/objects.py Outdated Show resolved Hide resolved
@pawamoy
Copy link
Member

pawamoy commented Apr 3, 2021

Thanks a lot @bstadlbauer!

Note that poetry environment setup is currently not possible due to a bug: python-poetry/poetry#534

Yes I saw that too yesterday and used the same fix in other projects 🙂

Your PR is LGTM! A nice fix with few changes 🙂
Would you like to me to fix CI so you can rebase? Or you're OK with merging immediately?

@bstadlbauer
Copy link
Contributor Author

It would be nice if you could fix CI cause I had six tests failing locally due to a weird import error :-)

Also, i hope it was ok to add marshmallow as a dev dependency?

@pawamoy
Copy link
Member

pawamoy commented Apr 3, 2021

Alright, lets fix that then! And yes, thank you for adding marshmallow, I guess I had it locally installed without Poetry knowing.

@pawamoy
Copy link
Member

pawamoy commented Apr 3, 2021

OK you can rebase on master to get the Poetry/pytest-randomly fix 🙂

@bstadlbauer bstadlbauer force-pushed the bugfix-duplicate-dataclass-attributes branch from b560e99 to 2386ab5 Compare April 3, 2021 12:02
@bstadlbauer
Copy link
Contributor Author

Rebased, but it looks as if the tests i saw failing locally are also failing here (and I think on master aswell).

Also, it seems as if code quality checking is exiting with a non-zero status code, but the test is marked as ok?

Run poetry run duty check-code-quality
  poetry run duty check-code-quality
  shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
  env:
    LANG: en_US.utf-8
    LC_ALL: en_US.utf-8
    POETRY_VIRTUALENVS_IN_PROJECT: true
    PYTHONIOENCODING: UTF-8
    pythonLocation: /opt/hostedtoolcache/Python/3.8.8/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.8.8/x64/lib
> Checking code quality
✗ Checking code quality (1)

@bstadlbauer
Copy link
Contributor Author

Note: Just checked and seems as if there are 125 linting errors on master at the moment:

➜  pytkdocs git:(master) ✗ poetry run flakehell lint src | wc -l 
     125

@pawamoy
Copy link
Member

pawamoy commented Apr 3, 2021

Don't pay attention to the check-code-quality results. I'm ignoring them indeed. The code still needs a big refactor and I didn't want to waste time on these.

@pawamoy
Copy link
Member

pawamoy commented Apr 3, 2021

However I'm not sure why the test suite doesn't pass anymore...
EDIT: hmmm I think pydantic is also missing from the dev-deps.

@bstadlbauer
Copy link
Contributor Author

Just just saw the same - adding it locally and testing again

@bstadlbauer
Copy link
Contributor Author

Works locally - added it here aswell, let's see if CI likes it

@bstadlbauer
Copy link
Contributor Author

Looks good :-) There is nothing more from my side, so ready to merge

@pawamoy
Copy link
Member

pawamoy commented Apr 3, 2021

Thanks again!

@pawamoy pawamoy merged commit 5a391e5 into mkdocstrings:master Apr 3, 2021
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