-
-
Notifications
You must be signed in to change notification settings - Fork 364
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 decorator for convert #240
Comments
I think it's mostly laziness on my part and a tad of indecisiveness because I'm a bit annoyed by the inconsistency of validator (noun) vs convert (verb). :-/ |
Just a follow up on this, I could also provide a PR similar to how validator is defined. perhaps as convertor to keep the naming consistent? Using a toy example, would something like this be accepted? ` @thing.convertor |
@devdave: I think so. We might have to bikeshed the name, but that's a smaller matter. I suggest you go with |
Went with dirt simple
Adjacent, I can't find where to make a basic test for the above method. Personally, at minimum I make a stub to act as pseudo documentation and also locking the signature into a project. Held off with submitting a PR to keep clutter down. |
I’m not sure I can parse this, are you asking where to put the test for the decorator? I’d say put behind the default decorator tests in test_make.py and another one int test_dark_magic.py. It should also be demonstrated on the examples page. |
Obviously not a pull request but initial changes are here. https://github.com/devdave/attrs/tree/pr_240 Is there an ideal PR I can use as a guiding example of what I need to aim for to make sure this would be accepted? I don't entirely understand hypothesis but get the impression it is a BDD like extension to pytest? |
Well the code is fine however you’re taking advantage of the fact that the current bad name is different from your decorator. :). IOW it has tech debt built right in and it would be nice if it could be done properly right away. If you want you can wait or tackle the convert → converter dance first. |
@hynek That's not too big of a problem. Starting on refactoring; instance property |
All tests passing with changes to _CountingAttr, Attribute, and the init building logic |
Please just open a pull request, it’s really hard to follow along like this. :) |
@devdave any chance of a PR? |
I think some more refactoring is required to allow the converter decorator to take self, like how default and validator work Using @devdave changes, it currently only works like this @thing.converter
def _thing_converter(value):
returned do_conversion(value) if ... else value |
So with #1267 merged, what is needed to move this feature forward? |
One can have
@validator
and@default
but no@convert
. Is this an oversight or by intention? If it's just an oversight, I'd be happy to send a PR for it.The text was updated successfully, but these errors were encountered: