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 and_ converter #618

Merged
merged 8 commits into from
Mar 5, 2020
Merged

Conversation

NI1993
Copy link
Contributor

@NI1993 NI1993 commented Jan 23, 2020

Pull Request Check List

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.

  • [V] Added tests for changed code.
  • [V] New features have been added to our Hypothesis testing strategy.
  • [V] Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
    • [V] ...and used in the stub test file tests/typing_example.py.
  • [V] Updated documentation for changed code.
    • [V] New functions/classes have to be added to docs/api.rst by hand.
    • [V] Changes to the signature of @attr.s() have to be added by hand too.
    • [V] Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
  • [V] Documentation in .rst files is written using semantic newlines.
  • [V] Changes (and possible deprecations) have news fragments in changelog.d.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

@NI1993 NI1993 force-pushed the feature/and_converter branch from 7b1403b to 36eef7e Compare January 23, 2020 17:11
:param converters: Arbitrary number of converters.
:type converters: callables

.. versionadded:: ???
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can one tell the versionadded?

Copy link
Member

Choose a reason for hiding this comment

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

It's in attr.__init__ without the .dev0 suffix.

@wsanchez
Copy link

Lint error:

tests/test_converters.py:13:1: F401 'attr.s' imported but unused

@wsanchez
Copy link

No tests for the and_converter nested function.

@hynek
Copy link
Member

hynek commented Jan 25, 2020

Looking at the code, I find the name and unfortunate. It's confusing by being the same as for validators but doing something different. I guess chain would work?

@NI1993
Copy link
Contributor Author

NI1993 commented Feb 20, 2020

Lint error:

tests/test_converters.py:13:1: F401 'attr.s' imported but unused

Thanks, removed it

@NI1993
Copy link
Contributor Author

NI1993 commented Feb 20, 2020

Looking at the code, I find the name and unfortunate. It's confusing by being the same as for validators but doing something different. I guess chain would work?

Certainly!
Thanks, renamed it

@NI1993
Copy link
Contributor Author

NI1993 commented Feb 20, 2020

No tests for the and_converter nested function.

I'm not sure how to test it :
Can u please explain or refer me so I can learn how?

@NI1993 NI1993 requested a review from hynek February 20, 2020 11:07
@NI1993 NI1993 requested a review from wsanchez February 20, 2020 11:29
Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

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

Looks good overall, please address what I've mentioned.

The answer to how to test a nested function: you find a way to exercise it. :) But I believe you do that now?

:param converters: Arbitrary number of converters.
:type converters: callables

.. versionadded:: ???
Copy link
Member

Choose a reason for hiding this comment

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

It's in attr.__init__ without the .dev0 suffix.

tests/test_converters.py Outdated Show resolved Hide resolved
src/attr/converters.py Outdated Show resolved Hide resolved
@NI1993
Copy link
Contributor Author

NI1993 commented Mar 1, 2020

Looks good overall, please address what I've mentioned.

The answer to how to test a nested function: you find a way to exercise it. :) But I believe you do that now?

Well I did, but it was mentioned that I missed testing the nested function + the codecov check failed so I just wondering how can I pass it

tests/test_converters.py Outdated Show resolved Hide resolved
tests/test_converters.py Outdated Show resolved Hide resolved
@NI1993 NI1993 force-pushed the feature/and_converter branch from 4857681 to 71b52a1 Compare March 4, 2020 16:40
@NI1993
Copy link
Contributor Author

NI1993 commented Mar 4, 2020

@hynek it seems I still miss some..
What I'm I missing here 🤔

@hynek
Copy link
Member

hynek commented Mar 5, 2020

Looks like either out-of-date branch or another codecov glitch.

I think we're done here, thank you for your patience!

@hynek hynek merged commit 55d5ef4 into python-attrs:master Mar 5, 2020
@NI1993
Copy link
Contributor Author

NI1993 commented Mar 5, 2020

@hynek Many thanks!

hynek added a commit that referenced this pull request Mar 6, 2020
* Add and_ converter

* update _make

* Refactored code according to review

* fixed some linting errors

* minor refactor

* fixed CR comments

* fixed CR comment #2

Co-authored-by: Hynek Schlawack <[email protected]>
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.

3 participants