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

[python-experimental] implement in operator for model classes #7637

Merged
merged 2 commits into from
Oct 9, 2020

Conversation

mks-m
Copy link
Contributor

@mks-m mks-m commented Oct 9, 2020

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

Allow to check for attribute presence in model using in operator.

Unless I'm missing something, right now the only way to correctly get an attribute which may not be present is this:

try:
    attr_value = instance.attr_name
except ApiAttributeError:
    attr_value = None

which is quite cumbersome.

@spacether

@auto-labeler
Copy link

auto-labeler bot commented Oct 9, 2020

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. This looks great!

@spacether spacether merged commit 8416fff into OpenAPITools:master Oct 9, 2020
@mks-m
Copy link
Contributor Author

mks-m commented Oct 9, 2020

@spacether relevant question - is there value in raising an exception instead of returning None for access of legit attribute that doesn't have value? now that it's possible to test for presence of an attribute, it's easy to differentiate between that and legit None value.

code i want to simplify is this:

value = instance.attr if 'attr' in instance else None
if value is not None:
    # do something

with default None it's just be:

if instance.attr is not None:
    # do something

thoughts?

@spacether
Copy link
Contributor

spacether commented Oct 9, 2020

In my opinion there is value. Attributes can be nullable which is different than being unset.
Let's say
attribute_a has value None if we access it we should get None
Now let's say attribute a is unset if we try to access it directly with model.attribute_a we should get some kind of exception. This lets us differentiate between these use cases.

@spacether
Copy link
Contributor

spacether commented Oct 9, 2020

If you want this functionality why not add a default missing input argument to a get method in ModelNormal and ModelComposd? That would make it work like python dict.get.
Then one could do model.get(param_name, default_value_if_missing)
Also though, what if I want a property named get? How would we support that use case?

Why not use model._data_store.get(name)?

@mks-m
Copy link
Contributor Author

mks-m commented Oct 9, 2020

Now let's say attribute a is unset if we try to access it directly with model.attribute_a we should get some kind of exception. This lets us differentiate between these use cases.

we definitely can differentiate in both cases via exception or by using 'attr' in instance, i just think that using exceptions for that leads to more complex code than necessary. it is much more often that one needs to react to None value regardless if it was set or the attribute is missing, than to differentiate between set and unset None.

Then one could do model.get(param_name, default_value_if_missing)

yes, that's another method i want to add, but it would be nice if using attributes directly was more convenient. right now i have to wrap in try/except way too much :(

Why not use model._data_store.get(name)?

i don't think all attributes are in there? required attributes seem to be in model.__dict__ directly.

@spacether
Copy link
Contributor

spacether commented Oct 9, 2020

i don't think all attributes are in there? required attributes seem to be in model.dict directly.

The attribute that are not in _data_store are data about the class or the composed instance.
I think that all payload key value pairs are in _data_store.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants