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

[2/🚀] Create feature flag allow-empty-values #7

Merged

Conversation

kyle-rader-msft
Copy link
Contributor

@kyle-rader-msft kyle-rader-msft commented Mar 1, 2024

This PR address #8, introducing our first feature flag to allow for parsing lines that have a key but no value.

We also enable running the tests with all features enabled as well as the default.

Questions

I'm unsure of what's a better representation for this: Line::Nothing or Pair("non-empty key", "")?

Base PR (#6)

This PR is based on (#6) and I will rebase once that is merged. until then, the diff will be that of both PRs.

@kyle-rader-msft kyle-rader-msft changed the title Create feature flag allow-empty-values [2/🚀] Create feature flag allow-empty-values Mar 1, 2024
@TheDaemoness
Copy link
Owner

I'm unsure of what's a better representation for this: Line::Nothing or Pair("non-empty key", "")?

Hmm, good question. I like Line::Pair here more on principle as an accurate reflection of the what the line says.
Currently Properties allows empty values to be stored in it, affecting the return value of Properties::len but otherwise only serving to bloat lookup times. One could argue this behavior is a bug 😉. I'll work on fixing it either this weekend or next.

@kyle-rader-msft
Copy link
Contributor Author

kyle-rader-msft commented Mar 4, 2024

@TheDaemoness , I agree that Pair preserves the most accurate representation, but like you pointed out, is this useful without a value, or is the same interpretation had from pretending it didn't exist at all :).

I've rebased this PR, changing the result to be the Pair with empty value. I also updated the CI to include a second run of the unit tests with the --all-features option enabled to make sure this is covered in CI.

I think your proposed change could be an optimization, but would also be fine without it and letting callers deal with allowing empty values to begin with. The feature flag sort of imples you will get empty values, rather than nonthing ;).

If you wouldn't mind cutting a release after this merges, that would be greatly appreciated!

@TheDaemoness
Copy link
Owner

Sounds good to me. I'll merge this and, as a temporary measure, alter ConfigParser to discard empty values if the feature flag is enabled, here:

ec4rs/src/parser.rs

Lines 89 to 91 in d469e3e

Ok(Line::Pair(k, v)) => {
section.insert(k, v.to_owned());
}

@TheDaemoness TheDaemoness merged commit ca62ab4 into TheDaemoness:main Mar 7, 2024
4 checks passed
@kyle-rader-msft kyle-rader-msft deleted the user/kyrader/empty-values branch March 7, 2024 18:00
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.

2 participants