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

Refactoring and normalized-method #123

Merged
merged 4 commits into from
Jul 13, 2015

Conversation

funkyfuture
Copy link
Member

The main purposes of this PR is to prepare the code to implement #93 (and therefore rely completely on child-validators in order to follow object's trails). Furthermore it contains a lot of code-cleanup and -consolidation. some methods have been renamed for the sake of clarification.

this isn't finished yet as i will try to reduce code even more regarding one issue.

in a later commit i would reorder the methods to an order as meaningful as possible.

@nicolaiarocci if this is supposed to be included not before the 1.0-release, i would also remove any deprecated parts. as i would also like to apply that to the documentation, an older version of the docs may be made available publicly.

@misja i don't see the point of the current-method and would like to remove it. any objections?

@funkyfuture
Copy link
Member Author

eventually i remodeled the context's aspects that imo names the things in a clearer way.

however, i think it may even be useful to make what is now Validator.context available as Validator.root_document and hold the immediate parent-mapping as Validator.context.

@funkyfuture funkyfuture force-pushed the refactoring branch 3 times, most recently from 790cc91 to 86776a5 Compare July 5, 2015 21:23
@funkyfuture
Copy link
Member Author

@nicolaiarocci can you estimate when you will review this? i'd appreciate any short feedback for my planning.

@funkyfuture
Copy link
Member Author

however, i think it may even be useful to make what is now Validator.context available as Validator.root_document and hold the immediate parent-mapping as Validator.context.

note to self: this may be easily tackled with a trail-property of Validator


def __call__(self, *args, **kwargs):
return self.validate(*args, **kwargs)

# TODO REMOVE? isn't used in the code,
# seems uninteresting for the public api
Copy link
Member

Choose a reason for hiding this comment

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

yup

@nicolaiarocci
Copy link
Member

Looks good to me - also a much needed refactor and cleanup work. We could merge this so normalized is available with 0.10. Cleanup of deprecated stuff can come up later, when 1.0 is released.

@funkyfuture
Copy link
Member Author

what about the reanimg of self._current to self.document and self.document to self.context. that could break custom validators and can't be aliased somehow.

i'd like to change that rather sooner than later as it clarifies a lot. or should i rather change this back and add comments as explanation and reminder?

@nicolaiarocci
Copy link
Member

That's one tough question. Since we're still in beta though, I'd go ahead with the change. Make sure changelog is properly updated. Maybe also add docstrings notes.

@funkyfuture
Copy link
Member Author

i named it root_document. that's unambigious.

but i didn't change it in test_self_document_always_root. and it doesn't fail. ?:-/

otherwise i'm fine with merging it.

- minor fix
- Limitations on customizing in the docs
@funkyfuture
Copy link
Member Author

i added a commit that reorders the methods and comes with a minor fix and docs extension.

can you think of a clearer name for Validator.update?

@funkyfuture funkyfuture force-pushed the refactoring branch 2 times, most recently from b69a0fb to 0b813da Compare July 11, 2015 22:34
also,
- fixed test for root_doc in custom validator
- adds annotations for missing code
- small docs-fix
@funkyfuture
Copy link
Member Author

but i didn't change it in test_self_document_always_root. and it doesn't fail. ?:-/
otherwise i'm fine with merging it.

fixed it. my bad.

@nicolaiarocci nicolaiarocci merged commit 9677ad9 into pyeve:master Jul 13, 2015
@funkyfuture funkyfuture deleted the refactoring branch July 16, 2015 22:52
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