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

Use Mapping as a default SchemaNode typ. #162

Closed
wants to merge 1 commit into from

Conversation

kotnik
Copy link
Contributor

@kotnik kotnik commented Jan 8, 2014

Typ of the Node should have a reasonable default, just like the documentation for _SchemaNode suggests.

@tseaver
Copy link
Member

tseaver commented Jan 8, 2014

Thanks for the patch! I am +0 on the substance of the patch, but could you please back out the janitorial changes (the "Remove trailing whitespace" commit, I think) to make the bits being reviewed clearer?

@kotnik
Copy link
Contributor Author

kotnik commented Jan 9, 2014

I have removed the irrelevant commit (my editor automatically removes the trailing garbage).

@jayd3e
Copy link
Contributor

jayd3e commented Feb 9, 2014

So the general consensus with this issue is that SchemaNode should always require a type in some form passed into it, b/c it is meant to be a lower level abstraction. MappingSchema/TupleSchema/SequenceSchema allow for this use-case, b/c you shouldn't have to pass the Mapping type to MappingSchema for example.

'Schema node construction without a typ argument or '
'a schema_type() callable present on the node class '
)
return Mapping
Copy link
Member

Choose a reason for hiding this comment

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

This should be an instance of Mapping. Specifically your options are:

@staticmethod
def schema_type():
    return Mapping()

or

schema_type = Mapping

@mmerickel
Copy link
Member

This PR needs a test.

@kotnik
Copy link
Contributor Author

kotnik commented Apr 16, 2014

Ok, I'll provide tests as well.

@mmerickel
Copy link
Member

FWIW I think that SchemaNode should actually not have a default type and the docs be updated to reflect that. As this PR currently stands I think it will have no effect because the default type is still taken from the first argument to SchemaNode.__init__ (even if the first argument is another SchemaNode for use with the default mapping type).

@mmerickel
Copy link
Member

We've decided to update the docs to indicate that the first parameter to SchemaNode is always the type and that there is no default so this patch is no longer necessary.

@mmerickel mmerickel closed this Jul 2, 2015
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

4 participants