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

config: No more '#' or ';' comments; use '//' instead #1687

Closed
wants to merge 2 commits into from

Conversation

dgw
Copy link
Member

@dgw dgw commented Sep 7, 2019

Any alternative idea that would let us keep using # for comments is welcome, but as described in the relevant commit message I don't think that's possible without some very dirty monkey-patching.

'Documentation' label because I'll have to make damn sure I include very prominent notes about this in the changelog and migration guide. If someone loads an old config with now-unrecognized comments, Sopel will explode with a very unhelpful traceback:

$ sopel
Traceback (most recent call last):
  File "/mnt/c/Users/dgw/github/sopel/sopel/config/types.py", line 55, in __init__
    getattr(self, value)
  File "/mnt/c/Users/dgw/github/sopel/sopel/config/types.py", line 384, in __get__
    this_section = getattr(main_config, instance._section_name)
  File "/mnt/c/Users/dgw/github/sopel/sopel/config/__init__.py", line 218, in __getattr__
    section = self.ConfigSection(name, items, self)  # Return a section
  File "/mnt/c/Users/dgw/github/sopel/sopel/config/__init__.py", line 187, in __init__
    value = item[1].strip()
AttributeError: 'NoneType' object has no attribute 'strip'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/bin/sopel", line 11, in <module>
    load_entry_point('sopel', 'console_scripts', 'sopel')()
  File "/mnt/c/Users/dgw/github/sopel/sopel/cli/run.py", line 680, in main
    return command(opts)
  File "/mnt/c/Users/dgw/github/sopel/sopel/cli/run.py", line 573, in command_legacy
    config_module = get_configuration(opts)
  File "/mnt/c/Users/dgw/github/sopel/sopel/cli/run.py", line 329, in get_configuration
    settings = utils.load_settings(options)
  File "/mnt/c/Users/dgw/github/sopel/sopel/cli/utils.py", line 338, in load_settings
    return config.Config(filename)
  File "/mnt/c/Users/dgw/github/sopel/sopel/config/__init__.py", line 119, in __init__
    validate=validate)
  File "/mnt/c/Users/dgw/github/sopel/sopel/config/__init__.py", line 173, in define_section
    setattr(self, name, cls_(self, name, validate=validate))
  File "/mnt/c/Users/dgw/github/sopel/sopel/config/types.py", line 65, in __init__
    value)
ValueError: Missing required value for core.ca_certs

(Side note: Manually cleaning up that copy-paste due to microsoft/terminal#1073 was fun.)

dgw added 2 commits September 7, 2019 16:21
If we allow '#' to start a comment, the `core.channels` option breaks.
IRC channel names almost always start with '#', so when Sopel saves its
channel list in the new multi-line format, the option will be empty on
next config load.

It's a shame. '#' is the most natural comment prefix in the world for a
Python app like Sopel (because Python itself uses that prefix), but
there's no way around this ConfigParser behavior without completely
replacing the internal file read method. We can't simply tell the parser
not to ignore comments if they're indented; it `strip()`s each line of
whitespace before checking.
@dgw dgw added High Priority Documentation Bugfix Generally, PRs that reference (and fix) one or more issue(s) labels Sep 7, 2019
@dgw dgw added this to the 7.0.0 milestone Sep 7, 2019
@dgw dgw requested review from Exirel and a team September 7, 2019 21:42
@dgw dgw changed the title Fix core.channels config: No more '#' or ';' comments; use '//' instead Sep 7, 2019
@dgw dgw added Replaced Superseded by a newer PR and removed High Priority labels Sep 9, 2019
@dgw dgw removed this from the 7.0.0 milestone Sep 9, 2019
@dgw dgw removed request for a team and Exirel September 9, 2019 12:49
@dgw
Copy link
Member Author

dgw commented Sep 9, 2019

Closing in favor of #1689. Besides the fact that losing # comments in config files would suck, this solution also just doesn't work on Python 2.7: The comment_prefixes argument to RawConfigParser is not available until Python 3.2.

@dgw dgw closed this Sep 9, 2019
@dgw dgw deleted the fix-core.channels branch September 9, 2019 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s) Documentation Replaced Superseded by a newer PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant