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

support other data type than string, add some failsafes #5

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

BigRoy
Copy link
Owner

@BigRoy BigRoy commented Mar 19, 2019

Update from current pypeclub:master

@BigRoy
Copy link
Owner Author

BigRoy commented Mar 19, 2019

@mkolar is this the latest version you are using in production?

I noticed #2 is still an open PR and never was merged - are you using that code? If I merge that it might break your usages so maybe we'll need to iterate on what changes make sense currently for acre? (That PR is part of #4 I believe)

@mkolar
Copy link

mkolar commented Mar 20, 2019

To be honest I need to double check with the team. I believe we tried to go back to your original code, but I lost track now. Will get back to you asap

Copy link
Owner Author

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

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

Some general notes about the code.

Comment on lines +47 to +57
try:
dependent_keys = re.findall("{(.+?)}", value)
for dependency in dependent_keys:
# Ignore direct references to itself because
# we don't format with itself anyway
if dependency == key:
continue

dependencies.append((key, dependency))
dependencies.append((key, dependency))
except Exception:
dependencies.append((key, value))
Copy link
Owner Author

@BigRoy BigRoy Mar 25, 2022

Choose a reason for hiding this comment

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

Why does this need to be a try..except? Doesn't make much sense to me.

That'd only occur if value is not a string maybe? E.g. a number or None? But that basically means the env isn't an "environment" so the input is already invalid to begin with? This should error. No?

Comment on lines +72 to +73
if not isinstance(env[key], str):
continue
Copy link
Owner Author

Choose a reason for hiding this comment

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

What kind of values are you expected in the input env. I'd say that anything that is not a string is not an "environment value" and thus is invalid input. That should not be continued but should raise an error. It means the input value is wrong.

Unless maybe we'd want to explicitly skip None values?

acre/lib.py Outdated
Comment on lines 49 to 58
except Exception:
r_token = re.compile(r"({.*?})")
matches = re.findall(r_token, s)
f = s
for m in matches:
try:
f = re.sub(m, m.format(**data), f)
except KeyError:
continue
return f
Copy link
Owner Author

Choose a reason for hiding this comment

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

Could we improve the Exception (e.g. more explicit; what exception should we capture?) and add a comment to the code as to what this is trying to solve? When should this exception occur?

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.

4 participants