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

Ignore fields parameter in assert methods #32

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nadaj
Copy link
Contributor

@nadaj nadaj commented Jan 27, 2018

Fixed issue: #21

Added parameter to assert methods that can ignore single or multiple fields (given as a list) in snapshot.

def remove_fields_from_dict(cls, dictionary, remove_fields=None):
if remove_fields is None:
remove_fields = []
for field in remove_fields:
Copy link

Choose a reason for hiding this comment

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

This won't work with nested fields, right?
something like

{
    "content": {
        "created_at": "......."
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, in this case it will not work. What do you suggest in this case since we can have a situation like this:

{
    "created_at": ... ,
    "content": {
        "created_at": "......."
    }
}

Should the remove_fields parameter perhaps be a list if we would want to separate these cases? In this example if you wanted to delete the non-nested created_at you could just pass 'created_at' or ['created_at'] and for the nested ['content', 'created_at'].

The second option would be to globally remove the given field, so if 'created_at' is given as the parameter, it should remove every appearance of it (including nested).

Copy link

Choose a reason for hiding this comment

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

Right. I'm not quite sure myself on what to do in this situation. One approach would be to specify not just keys but the whole path, kind of like "content.items.*.event.created_at" however, for my needs, I just forked this repo and made it delete every key that matches the name recursively.

@ish-vasa
Copy link

ish-vasa commented Feb 5, 2019

@bsod90 @nadaj --> any plans to push this through or can i pick it up?

@josephtyler
Copy link

This would be a nice feature to have.

@HeyHugo
Copy link

HeyHugo commented Oct 21, 2019

Nice!

It would be nice if you still asserted the keys exist before removing them. I.e we ignore what is rendered for the ignored fields but still assert that they are present.

@gurunars
Copy link

gurunars commented Feb 3, 2020

Duplicates this one: #102

And I would say that it should be treated similarly.

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.

6 participants