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

fix(conf) overcome penlight's 'list_delim' setting #1569

Merged
merged 1 commit into from
Aug 30, 2016

Conversation

thibaultcha
Copy link
Member

Penlight's parsing would infer some settings as tables because of their
format, and our config parser would then contain tables values instead
of strings, which would make tests such as value == "on" obsolete and
would not correctly infer our booleans.

  • add list_delim placeholder for config parsing
  • move trailing comment removal earlier in the check_and_infer function
  • add tests for dnsmasq = off and custom_plugins file parsing

Penlight's parsing would infer some settings as tables because of their
format, and our config parser would then contain tables values instead
of strings, which would make tests such as `value == "on"` obsolete and
would not correctly infer our booleans.
@Tieske
Copy link
Member

Tieske commented Aug 29, 2016

lgtm

@thibaultcha thibaultcha added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/status/needs review labels Aug 30, 2016
@thibaultcha thibaultcha merged commit 185fc64 into next Aug 30, 2016
@thibaultcha thibaultcha deleted the fix/conf-parsing branch August 30, 2016 02:01
@WALL-E
Copy link
Contributor

WALL-E commented Aug 31, 2016

from_file_conf, err = pl_config.read(s, {
  smart = false,
  list_delim = "_blank_" -- mandatory but we want to ignore it
})

_blank_: Is only used to ignore this feature(list_delim), should not be used in the configuration file

Do I understand this correctly? @thibaultcha

@Tieske
Copy link
Member

Tieske commented Sep 1, 2016

@WALL-E yes, the parser requires that value in its options, but it should never be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants