Skip to content

Modified owners validator to accept dict#847

Merged
IDoneShaveIt merged 3 commits intoelementary-data:masterfrom
manulpatel:ele-716-owner-dict
May 8, 2023
Merged

Modified owners validator to accept dict#847
IDoneShaveIt merged 3 commits intoelementary-data:masterfrom
manulpatel:ele-716-owner-dict

Conversation

@manulpatel
Copy link
Copy Markdown
Contributor

@manulpatel manulpatel commented May 4, 2023

Fixes #827

Modified load_owners validator to accept and return dict. This would prevent report failure.

Copy link
Copy Markdown
Contributor

@IDoneShaveIt IDoneShaveIt left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!
Left some small comments 🙂

return var
elif isinstance(var, str):
loaded_var = try_load_json(var)
if type(loaded_var) is dict:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if type(loaded_var) is dict:
if isinstance(loaded_var, dict):

Match the rest of the method type checking 🙂

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Noted

Comment on lines +28 to +29
loaded_var = json.dumps(loaded_var)
return loaded_var
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
loaded_var = json.dumps(loaded_var)
return loaded_var
loaded_var = [json.dumps(loaded_var)]

@staticmethod
def _load_var_to_list(var: Union[str, list]) -> list:
if not var:
if not var & type(var) != dict:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if not var & type(var) != dict:
if not var:

I don't think this one should be changed.
Currently we support Union[str, list] so if someone is passing a dict it should be OK to fail 🙂.

Copy link
Copy Markdown
Contributor

@IDoneShaveIt IDoneShaveIt left a comment

Choose a reason for hiding this comment

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

Looking good!
Thank you for the contribution 💪🏻 💪🏻

@IDoneShaveIt IDoneShaveIt merged commit afd2928 into elementary-data:master May 8, 2023
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.

[ELE-716] Report fails to generate if owner is configured as dict (exposures format)

2 participants