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 Schema.remove method to remove keys from dict (opposite of extend). #325

Closed
wants to merge 1 commit into from

Conversation

benjaminweb
Copy link

If there is extend method, why not go the other way around?

@coveralls
Copy link

coveralls commented Feb 5, 2018

Coverage Status

Coverage decreased (-0.6%) to 94.607% when pulling 749dd94 on benjaminweb:master into aa29b08 on alecthomas:master.

@@ -399,6 +399,30 @@ def test_schema_extend_key_swap():
assert_true((list(extended.schema)[0], Required))


def test_remove():
Copy link
Collaborator

Choose a reason for hiding this comment

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

@benjaminweb There is also a function called test_remove on line 91 so one of these tests isn't running.

@mprager
Copy link

mprager commented Feb 7, 2018

I'm not sure this will make it through. I had a request like this one rejected awhile ago: #261

@svisser
Copy link
Collaborator

svisser commented Feb 7, 2018

It may be useful to have an example of when this would be used.

I can imagine that the keys to remove are only known at runtime but I haven't come across that need myself.

@benjaminweb
Copy link
Author

renamed test_remove to test_remove_method

@svisser what would you add to the basic example in the test? Any specific you have on your mind? ;)

@benjaminweb
Copy link
Author

@mprager Do you have any idea for a convincing example? Would this justify the pull request?

@alecthomas
Copy link
Owner

Each piece of functionality we include adds complexity. For solutions to common problems, that's justified, but this is not such a case IMO (it is extremely niche) and it can be solved by refactoring your schemas (similarly to how one would factor out a common base class of a class inheritance hierarchy) or with an external helper function.

Thanks for the contribution though.

@alecthomas alecthomas closed this Feb 9, 2018
@benjaminweb
Copy link
Author

@alecthomas thanks for your consistency in pursuing simplicity! Appreciated.

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

5 participants