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

Restore the ability to use inline comments in config files #461

Merged

Conversation

neilmayhew
Copy link
Contributor

@neilmayhew neilmayhew commented Jan 31, 2020

This change is Reviewable

@neilmayhew neilmayhew force-pushed the fix/inline-config-comments branch from 4f7e021 to e671acd Compare January 31, 2020 22:02
@JoeLametta
Copy link
Collaborator

JoeLametta commented Feb 3, 2020

Hi, one clarification: you titled "restore" but when did we drop the ability to use inline comments? (in which commit?).

@neilmayhew neilmayhew force-pushed the fix/inline-config-comments branch 2 times, most recently from e7ce6a5 to 6ebc711 Compare February 3, 2020 15:58
@neilmayhew
Copy link
Contributor Author

I added more info in the commit message, and also updated README.md. The exact commit where the ability was lost was 64dd9d8 but I'm not sure this is important to include in the commit message.

@JoeLametta
Copy link
Collaborator

JoeLametta commented Feb 3, 2020

Here I see this:

(For backwards compatibility, only ; starts an inline comment, while # does not.)

Also keep in mind that inline comments may cause interpolation errors. Example here: #443 (comment).

Um, don't know if restoring this would be a good idea...

@neilmayhew
Copy link
Contributor Author

I suspect that #443 is actually a result of the problem this PR is trying to fix, because the comments won't have been treated as comments in any Python 3 version of whipper: they'll just have been part of the value. In other words, re-enabling inline comments would prevent that problem.

Without this PR, inline comments won't be possible at all, but they're used liberally in whipper's documentation and examples, so those places would all need to be updated.

However, more importantly, I expect many people (like me) have existing config files with comments in them that were used with a Python 2 version of whipper, and now whipper dies with a strange error message about boolean values whenever they try to run it.

@neilmayhew
Copy link
Contributor Author

I'm not sure what you meant by the quote from the Python 2 documentation. That if we do re-enable comments we should allow only ; and not #?

@JoeLametta
Copy link
Collaborator

JoeLametta commented Feb 3, 2020

Hi, after some tests I've verified that:

  1. before the Python 3 port occurred whipper used to recognize inline comments starting with character ; but not the ones with character #. This seems to be confirmed here too †
  2. right now whipper allows no inline comments

I agree this is a regression so I'm going to merge this pull request but first I need you to edit it to remove character # as allowed inline comment prefix.

Cheers,
Joe

Changed in version 3.2: In previous versions of configparser behaviour matched
comment_prefixes=('#',';') and inline_comment_prefixes=(';',).

The ability was lost in the switch to Python 3, because the config
parser module in the standard library changed its defaults.

[Python 2][2]:

> Comments may appear on their own in an otherwise empty line, or
> may be entered in lines holding values or section names.

[Python 3][3]:

> Inline comments can be harmful because they prevent users
> from using the delimiting characters as parts of values.
> That being said, this can be customized.

[2]: https://docs.python.org/2/library/configparser.html#module-ConfigParser
[3]: https://docs.python.org/3/library/configparser.html#supported-ini-file-structure

Signed-off-by: Neil Mayhew <[email protected]>
@neilmayhew neilmayhew force-pushed the fix/inline-config-comments branch from 6ebc711 to dca9fcb Compare February 3, 2020 22:02
@neilmayhew
Copy link
Contributor Author

Yes, I agree it's best to match the original behaviour exactly. Done.

@JoeLametta JoeLametta merged commit 3213241 into whipper-team:develop Feb 4, 2020
@JoeLametta
Copy link
Collaborator

Merged, thanks!

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