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 scope remover processor #132

Merged
merged 4 commits into from
Sep 7, 2017

Conversation

jkakavas
Copy link
Member

@jkakavas jkakavas commented Aug 8, 2017

Adds a processor for the AttributeProcessor microservice that removes the scope from scoped attributes

for value in attributes.get(attribute, [None]):
if '@' in value:
new_values.append(value.split('@')[0])
attributes[attribute] = new_values
Copy link
Member

Choose a reason for hiding this comment

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

This will remove any attribute values without a @, and leave only values that had a scope (but with the scope-part removed). This is not clear from the class name.

If this was not intentional you only need to remove the if '@' in value clause (I'm ignoring the problem described in the comment above)

for value in attributes.get(attribute, [None]):
  value_with_no_scope = value.split('@')[0]
  new_values.append(value_with_no_scope)

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is that the deployer uses this microservice for attributes that are scoped. I agree though that removing the attribute entirely in case it is not scoped, doesn't make much sense and will just add to confusion. I will revisit this

def process(self, internal_data, attribute, **kwargs):
attributes = internal_data.attributes
new_values = []
for value in attributes.get(attribute, [None]):
Copy link
Member

Choose a reason for hiding this comment

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

If attribute is not found, [None] will be returned and value will be set to None. Then value.split() will fail. We should probably check if the attribute is there, and then process the values. If attribute is not there, I would raise an AttributeProcessorWarning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

new_values = []
values = attributes.get(attribute, [None])
if values:
for value in attributes.get(attribute, [None]):
Copy link
Member

Choose a reason for hiding this comment

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

You already have values set, no need to get() from attributes again ;)


btw, if you want to make this "prettier", you can invert the conditional and remove the else clause and indentation

if not correct:
  raise SomeErrror # processing will stop here

# "implied" else can go here now
do_stuff(...)
do_other_stuff(...)

This also moves the error handling closer to the cause, which makes things a bit more readable.

@johanlundberg johanlundberg merged commit fe7a267 into IdentityPython:master Sep 7, 2017
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.

3 participants