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

Fix ignore fields #89

Merged
merged 1 commit into from
Dec 14, 2021
Merged

Fix ignore fields #89

merged 1 commit into from
Dec 14, 2021

Conversation

canassa
Copy link

@canassa canassa commented Dec 11, 2020

  • Do not check if both the original and camel case fields are in the ignore
    list. Checks only if the original field is there.
  • Fixes the "else" clause.

Fixes #86

- Do not check if both the original and camel case fields are in the ignore
  list. Checks only if the original field is there.
- Fixes the "else" clause.
@canassa canassa changed the title 🐛 fix ignore fields WIP: fix ignore fields Dec 11, 2020
@canassa canassa changed the title WIP: fix ignore fields Fix ignore fields Dec 11, 2020
@@ -37,10 +37,10 @@ def camelize(data, **options):
new_key = re.sub(camelize_re, underscore_to_camel, key)
else:
new_key = key
if key not in ignore_fields and new_key not in ignore_fields:

Choose a reason for hiding this comment

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

I think this is there for a reason, it's so you can add either the (prospective) new key, or the old key, to a list of keys to ignore.

Copy link
Author

@canassa canassa Feb 6, 2021

Choose a reason for hiding this comment

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

hmm, if that's the case I would still rewrite it as:

if key in ignore_fields or new_key in ignore_fields:
     new_dict[key] = value
else:
    new_dict[new_key] = camelize(value, **options)

It's a bit easier to read IMO

new_dict[new_key] = camelize(value, **options)
else:
new_dict[new_key] = value
new_dict[key] = value

Choose a reason for hiding this comment

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

This change (and this change alone) will fix the issue. It will break the tests however, please see

"ignore_me": {"noChangeRecursive": 1},

@djowett-ftw
Copy link

@stevewilliamsuk @canassa could I invite you to take a look at (and even review) #93 as an alternative solution here? It adds a new option as ignore_fields was designed to still change the key. The new option uses the name suggested by @vbabiy.

@vbabiy vbabiy merged commit d5b8574 into vbabiy:master Dec 14, 2021
@vbabiy
Copy link
Owner

vbabiy commented Dec 14, 2021

I had to revert this merge, due to failing tests. I need to understand what's going on here more closely.

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.

Ignore fields does not work as expected
4 participants