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

Allow optional conversion #173

Merged
merged 9 commits into from May 10, 2017
Merged

Conversation

adetokunbo
Copy link
Contributor

Fixes #105

... or at least, provides the simplest possible solution to #105.

I.e,
-it adds a simple callable that makes existing converters optional, using the code suggested by @Tinche
-it does not provide a single callable for both converters and validators . Please let me know if that's necessary

I went through the checklist; the only thing that I think is not done is the addition to the hypothesis testing strategy. I'm submitting and asking about that ;)

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!

  • Added tests for changed code.
  • New features have been added to our Hypothesis testing strategy.
  • Updated documentation for changed code.
  • Documentation in .rst files is written using semantic newlines.
  • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
  • Changes (and possible deprecations) are documented in CHANGELOG.rst.

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!

This just adds a function that makes an existing converter work for optional
inputs.
@codecov
Copy link

codecov bot commented Mar 29, 2017

Codecov Report

Merging #173 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #173   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           8      9    +1     
  Lines         546    554    +8     
  Branches      121    122    +1     
=====================================
+ Hits          546    554    +8
Impacted Files Coverage Δ
src/attr/converters.py 100% <100%> (ø)
src/attr/__init__.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d02b1d8...0b7aebe. Read the comment docs.

@adetokunbo
Copy link
Contributor Author

a review is required. How does a reviewer get assigned ?

@hynek
Copy link
Member

hynek commented Apr 4, 2017

Sorry no one got around to look at this yet! I'm currently busy preparing for a conference this weekend so my FLOSS activity is limited. I usually don't assign reviewers without good reason since that would be assigning work.

But In this case I'd like to ask @Tinche if this is what he had in mind before I review it once I have a free minute. If he does a full review: even better. ;)

We try to keep the PR queue as empty as possible; sorry you got caught in a bad moment!

@Tinche
Copy link
Member

Tinche commented Apr 4, 2017

Yeah, usually PRs adding features are lower priority than bug fixes and I've been busy too :)

The code LGTM, except we now have two hooks named optional, in validators and converters. I guess this is fine, folks can do import as.

@hynek
Copy link
Member

hynek commented Apr 14, 2017

So I looked at it and gotta say that this is annoying me WRT #146. No criticism to you @adetokunbo the code looks good! But before merging this, we should make a decision on #146 grumble.

@hynek hynek merged commit fdfd51e into python-attrs:master May 10, 2017
@hynek
Copy link
Member

hynek commented May 10, 2017

Thank you and sorry for the ultimately unnecessary delay.

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

3 participants