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

int -> str is added to allowed casts to overcome int to str incompatibility #35

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

Conversation

InnovArul
Copy link

Fix for #26

@InnovArul InnovArul changed the title int -> str is added to allowed casts to overcome int to str compatibility int -> str is added to allowed casts to overcome int to str incompatibility Dec 28, 2019
@muzammil360
Copy link

Great work @InnovArul.
@rbgirshick, any ideas when can this branch be merged and pip package updated?

@Rizhiy
Copy link
Contributor

Rizhiy commented Jan 1, 2020

I believe this is a wrong fix for my issue as it won't work in all cases, e.g. value "+2" will be converted to "2" in the end, thus losing the +.

The correct fix in my opinion would be to pass target type into _decode_cfg_value and don't use eval if target type is str.

I will try to code it up tomorrow, if I have the time.

@Rizhiy Rizhiy mentioned this pull request Jan 1, 2020
@InnovArul
Copy link
Author

Thanks for the heads up with this example. I have changed the _decode_cfg_value to handle this. Can you check if this handles your scenario?

@muzammil360
Copy link

@InnovArul @Rizhiy, how about you get configuration type from the default config value and then force that on incoming value!

@InnovArul
Copy link
Author

InnovArul commented Jan 3, 2020

@muzammil360 Such validation is already built-in to yacs with the method _check_and_coerce_cfg_value_type (here). But the issue is that, every string is passed through literal_eval (here) which changes +123 to 123. Now, I have changed the code to not to do literal_eval if the data type of the input as well as the expected datatype is a string.

@muzammil360
Copy link

muzammil360 commented Jan 3, 2020 via email

Larry-u added a commit to Larry-u/yacs that referenced this pull request Mar 10, 2021
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