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: quote ListAttribute items when required #1690

Merged
merged 1 commit into from
Oct 23, 2019

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Sep 10, 2019

In a ListAttribute, a value can start with a #, which is then considered as a comment by the Config reader. This is bad news, as channel names usually start with a # (they can start with a & too, which is less common on freenode).

To prevent this, what I do is:

  • quote the item only if necessary
  • unquote it if it correspond to a very specific pattern "#<name>"

This way, we can have this ListAttribute:

channels =
    "#sopel"
    &private

Edge case like "&something" won't be unquoted, because they don't match the exact pattern!

@dgw it replaces your PR #1689 because my first idea wasn't really on point. It would lead to a myriad of edge cases and unforeseen consequences. This one is, I believe, better, and doesn't change how current config are written.

@Exirel Exirel added the Bugfix Generally, PRs that reference (and fix) one or more issue(s) label Sep 10, 2019
@Exirel Exirel added this to the 7.0.0 milestone Sep 10, 2019
@Exirel Exirel requested a review from dgw September 10, 2019 22:26
@dgw
Copy link
Member

dgw commented Sep 19, 2019

./sopel/config/types.py:307:39: F812 list comprehension redefines 'value' from line 277, but only on Py2.7, because some scoping rule changed from 2 to 3.

@Exirel
Copy link
Contributor Author

Exirel commented Sep 20, 2019

@dgw fixed, no problem. :)

@Exirel
Copy link
Contributor Author

Exirel commented Sep 20, 2019

@dgw fixed, no problem. :)

Apparently there is something wrong with it: tests are failing for 2.7 only.

lol, well done Python 2.7, you're officially the worst.

@Exirel
Copy link
Contributor Author

Exirel commented Sep 20, 2019

OK I found the problem, it's from Python 2.7's ConfigParser built-in module:

if line.strip() == '' or line[0] in '#;':
    continue

So, if the line is empty: skip it.
If the line starts with # or ;: skip it.
If the line starts with anything else: do not skip.

On the other hand in Python 3, and I quote the documentation:

Comments can be indented.

My mood right now:

ezgif com-video-to-gif

(This is a gif by Boulet on Twitter, a French artist that also provides an English version of his blog)

@Exirel Exirel force-pushed the fix-listattribute-comments branch from cb3be11 to c51c397 Compare September 20, 2019 13:30
@Exirel
Copy link
Contributor Author

Exirel commented Sep 20, 2019

NOW it works.

@dgw I waited for the tests this time.

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little nemesiS here, bit of grammar there, and a technical question just 'cause. 😸

Why it took me so long to actually review this, I can't say. Thought I'd done so already. 😕

sopel/config/types.py Outdated Show resolved Hide resolved
sopel/config/types.py Outdated Show resolved Hide resolved
sopel/config/types.py Outdated Show resolved Hide resolved
sopel/config/types.py Show resolved Hide resolved
sopel/config/types.py Show resolved Hide resolved
sopel/config/types.py Outdated Show resolved Hide resolved
sopel/config/types.py Outdated Show resolved Hide resolved
@Exirel
Copy link
Contributor Author

Exirel commented Oct 23, 2019

@dgw will squash when approved! :)

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the applied review changes, all good to squash!

@Exirel Exirel force-pushed the fix-listattribute-comments branch from 2dbc47a to 05c0c07 Compare October 23, 2019 17:16
@Exirel
Copy link
Contributor Author

Exirel commented Oct 23, 2019

Waiting for Travis to do its job after the rebase & squash. 👍

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why. Did. I. Miss. These?! 😑

test/test_config.py Outdated Show resolved Hide resolved
test/test_config.py Outdated Show resolved Hide resolved
test/test_config.py Outdated Show resolved Hide resolved
test/test_config.py Outdated Show resolved Hide resolved
test/test_config.py Outdated Show resolved Hide resolved
test/test_config.py Outdated Show resolved Hide resolved
@dgw
Copy link
Member

dgw commented Oct 23, 2019

@Exirel Looks good. Squash again and I'll merge before I have any time to look for typos a third time. 😝

In a ListAttribute, a value can start with a #, which is then considered
as a comment by the Config reader. This is bad news, as channel names
usually start with a # (they can start with a & too, which is less
common on freenode).

To prevent this, what I do is:

* quote the item only if necessary
* unquote it if it correspond to a very specific pattern `"#<name>"`

This way, we can have this ListAttribute:

    channels =
        "#sopel"
        &private

Edge case like `"&something"` won't be unquoted, because they don't
match the exact pattern!

(with applied suggestions from code review)

Co-Authored-By: dgw <[email protected]>
@Exirel Exirel force-pushed the fix-listattribute-comments branch from 9836efb to 5dd7b8f Compare October 23, 2019 17:37
@Exirel
Copy link
Contributor Author

Exirel commented Oct 23, 2019

Done! :shipit:

@dgw dgw merged commit 5755a11 into sopel-irc:master Oct 23, 2019
@Exirel Exirel deleted the fix-listattribute-comments branch January 14, 2020 15:01
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) High Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants