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

748: handle obj.get(k, default) call pattern for backward compatibility #755

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lindsay-stevens
Copy link
Contributor

Closes #748

Why is this the best possible solution? Were any other approaches considered?

  • Allows external libs to continue using obj.get(key, default) with SurveyElement if needed
  • Encourages migration to new (or alternative) call patterns with ignorable DeprecationWarning
  • Tests demonstrate expected behaviour for range of call patterns, for clarification

Alternatives

The original code in #748 meant that elem.get("foo") would return None. This seemed seemed like it would be confusing for pyxform maintenance since a) there is still some elem.get("foo") code, and b) its easier to only be concerned about AttributeErrors (as would be expected for a non-dict class) rather than a mix of None, KeyError, AttributeError. If calling code like elem.get("foo") expects None then it can be adapted to use an explicit None default e.g. elem.get("foo", None) - otherwise it will raise an AttributeError as before. The SurveyElement also differs from dict in elem["foo"] (AttributeError not KeyError, respectively) so it didn't seem appropriate to fully replicate dict behaviour in this custom Mapping class.

Other alternatives mentioned in #748 and the warning message, for external libs to implement:

  • find-and-replace to swap obj.get(key, default) to getattr(obj, key, default)
  • checking the type with isinstance before accessing a potentially non-existent attribute
  • try/catch for AttributeError if the attribute doesn't exist
  • create a custom subclass that implements get in the preferred way

What are the regression risks?

There are no internal calls like obj.get(key, default). External code should work as before, except for the DeprecationWarning which can be ignored as shown in the tests, i.e. add:

warnings.simplefilter("ignore", DeprecationWarning)

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • included test cases for core behavior and edge cases in tests
  • run python -m unittest and verified all tests pass
  • run ruff format pyxform tests and ruff check pyxform tests to lint code
  • verified that any code or assets from external sources are properly credited in comments

- not used internally but for external libs that assume Mapping == dict
- test case notes other differences from default dict behaviour
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.

1 participant