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

We are converting empty string to None type #146

Closed
Johk3r opened this issue Feb 21, 2022 · 6 comments
Closed

We are converting empty string to None type #146

Johk3r opened this issue Feb 21, 2022 · 6 comments

Comments

@Johk3r
Copy link
Contributor

Johk3r commented Feb 21, 2022

elif node.lower() in {"", "null", "none"}:

Is it possible to change this part in order to get empty string read as string and not converted to None type?
I've added a PR with this change applied globally, as another option it could also be optional configuration,
ex.:
ParseIt( empty_string_value="" | None )

@Johk3r Johk3r mentioned this issue Feb 21, 2022
@naorlivne
Copy link
Owner

Hi @Johk3r , my feeling is that an empty string should count as None in default as some assumptions must be taken and that's what most people will think of an empty string to count as, I'm open to having it be configurable with the default as is and in other cases the end user can define what values in

elif node.lower() in {"", "null", "none"}:
count as None

Can you please describe your usecase/why is this requested?

@Johk3r
Copy link
Contributor Author

Johk3r commented Feb 22, 2022

HI @naorlivne, we use your library in our internal tool to interact with AWS cloud and terraform. Manage stack updates, etc.
I'm reading .hcl (terragrunt/terraform config files) and writing them back. Until now I've been able to work around this behavior by writing back all None types as empty strings, I can also write all None types as nulls. In reality they should store their original type as they behave differently.
Some examples:

  • Complex objects can't be set to an empty string, only to null, which would break if set to ""
  • Some of our code does checks like var.something == "" ? this : that, which would break if set to null
    So at least in the .hcl world I think it makes sense to have this distinction.
    Thank you :)

@naorlivne
Copy link
Owner

Changing the current default won't work, while I see your use case there are others said change will break so we'll really be replacing one issue with another.

I'm thinking something like having a configurable variable which by default is equal to {"", "null", "none"} which will be used in

elif node.lower() in {"", "null", "none"}:
will be the right approach, that way you (and other users) can change it to remove the empty strings while other who desires having strings like "empty" added will also be able to add their own.

This will require a bit more work though:

  • The variable will need to be added as a global variable to the ParseIt class with the default above
  • Adding said variable to the estimate_type function and all it's calls
  • Unit tests to cover the cases where the variable is used as default & cases it's changed
  • Documentation of the variable added to the readme file

If you feel like making a PR to that affect I'll gladly merge it in.

@Johk3r
Copy link
Contributor Author

Johk3r commented Feb 22, 2022

Sounds good, I'll try to get a PR with the change.
Thank you!

@Johk3r
Copy link
Contributor Author

Johk3r commented Feb 22, 2022

Have the PR ready: #148
Happy to change anything needed, this give us a starting point at least.

@Johk3r Johk3r mentioned this issue Feb 22, 2022
@Johk3r
Copy link
Contributor Author

Johk3r commented Feb 23, 2022

I think this is solved and can be closed.
Thanks to @naorlivne for helping getting this sorted out.

@Johk3r Johk3r closed this as completed Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants