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: split ListAttribute values on newlines #1628

Merged
merged 6 commits into from
Sep 5, 2019

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented May 30, 2019

See the description of #1626 for more information.

Note: I had to upgrade the test_config.py file to use py.test instead of unittest. We should be fine with that.

@Exirel Exirel added this to the 7.0.0 milestone May 30, 2019
@dgw
Copy link
Member

dgw commented May 30, 2019

Note: I had to upgrade the test_config.py file to use py.test instead of unittest. We should be fine with that.

Vaguely remembered noticing that unittest was used in only that one file… Found where I referenced it over a year ago: #1299 (comment) (knew I should have made an issue; it did, indeed, get forgotten…)

So, 🎉 for nixing that one last mismatched test framework. 😀

@Exirel Exirel force-pushed the config-regex-list-attribute branch from 4cae19a to 3bbdd58 Compare June 3, 2019 18:17
HumorBaby added a commit to HumorBaby/sopel that referenced this pull request Aug 28, 2019
Adds a setting in `core` called `commands_on_connect` based on the updated
`ListAttribute` from sopel-irc#1460. This setting stores a comma separated list
of raw IRC commands (`\`-escaped commas in commands) to execute upon
successful connection to the server. As @dgw put it, "Think ZNC's
perform module, but without the ability to add/remove/rearrange lines
from an IRC query" in sopel-irc#1455.

The commands in the `commands_on_connect` list are executed at the end of
the `startup` procedure. They can also be called by a bot admin with the
`.execute` command.

Note: two `TODO`s were added to adjust docstrings and docs once sopel-irc#1628 is
accepted, since it will change the `ListAttribute` delimiter from
commas to newlines.

Closes sopel-irc#1455
@dgw dgw changed the title config: separate list options by breaklines config: split ListAttribute values on newlines Aug 29, 2019
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.

It's long overdue that I looked at this. First order of business is to replace all "breaklines" with "newlines", because that's the word in English. (We could also use "line breaks", but why use lot word when few word do trick?) This included retitling the PR, and should also be reflected in the commit log when you update things.

Documentation-wise, I'm suggesting that the example ListAttribute be named in the plural ("cheeses"), along with all the potential whitespace changes that requires. This includes matching changes to the tests, for consistency.

Also expect the usual small tweaks to grammar and formatting. 😁

The actual logic change here is tiny, and doesn't need much attention; you'll note that all of my suggestions apply to documentation or tests, because those make up the bulk of this PR.

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 Outdated Show resolved Hide resolved
sopel/config/types.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
sopel/config/types.py Outdated Show resolved Hide resolved
@Exirel Exirel force-pushed the config-regex-list-attribute branch 2 times, most recently from 58b2ea7 to b9c862a Compare August 31, 2019 13:34
Exirel and others added 3 commits August 31, 2019 15:36
In Sopel 6.x and prior version, multi-line values would be split by
commas:

    option = one, two, three
             and a half, four

would give that:

    ['one', 'two', 'three\nand a half', 'four']

Also, it isn't possible to have a comma inside a value. However, a
fellow Sopelunker, in sopel-irc#1460, added an escape mechanism.

It was a good idea that didn't account for what Python's ConfigParser
could do for us. Indeed, it accepts, as a built-in behavior, to declare
an option on multiple lines. Getting the value would then returns a
string, strip from extra spaces, with values separated by newlines.

Therefore, all I had to do here was to:

* detect if there was a newline character in the value
* if so, split by newlines
* otherwise keep using the comma-separated approach
* remove (sadly) the usage of escape character
* add some tests
* improve docstrings here and there (they were outdated)
* make sure we properly serialize back the values

and voilà!
Co-Authored-By: dgw <dgw@technobabbl.es>
@Exirel Exirel force-pushed the config-regex-list-attribute branch from b9c862a to 522d400 Compare August 31, 2019 13:37
@Exirel
Copy link
Contributor Author

Exirel commented Aug 31, 2019

So, I had to rebase from master because there was several differences in requirements and in documentations needed... and that messed up the commit history in Github.

Talking about documentation, I'll now update it!

@Exirel
Copy link
Contributor Author

Exirel commented Aug 31, 2019

@dgw done!

@Exirel Exirel requested a review from dgw August 31, 2019 13:56
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.

Only a couple comments this time around.

sopel/config/types.py Outdated Show resolved Hide resolved
sopel/config/types.py Outdated Show resolved Hide resolved
@dgw
Copy link
Member

dgw commented Sep 1, 2019

Note to self: Remove #1460 from changelog/milestone when merging this for 7.0, as this PR reverts it.

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.

I think that's everything. Let's 🚢 it! 🚀

@dgw dgw merged commit bdc66f5 into sopel-irc:master Sep 5, 2019
Exirel pushed a commit to Exirel/sopel that referenced this pull request Dec 9, 2019
Adds a setting in `core` called `commands_on_connect` based on the updated
`ListAttribute` from sopel-irc#1460. This setting stores a comma separated list
of raw IRC commands (`\`-escaped commas in commands) to execute upon
successful connection to the server. As @dgw put it, "Think ZNC's
perform module, but without the ability to add/remove/rearrange lines
from an IRC query" in sopel-irc#1455.

The commands in the `commands_on_connect` list are executed at the end of
the `startup` procedure. They can also be called by a bot admin with the
`.execute` command.

Note: two `TODO`s were added to adjust docstrings and docs once sopel-irc#1628 is
accepted, since it will change the `ListAttribute` delimiter from
commas to newlines.

Closes sopel-irc#1455
Exirel pushed a commit to HumorBaby/sopel that referenced this pull request Dec 10, 2019
Adds a setting in `core` called `commands_on_connect` based on the updated
`ListAttribute` from sopel-irc#1460. This setting stores a comma separated list
of raw IRC commands (`\`-escaped commas in commands) to execute upon
successful connection to the server. As @dgw put it, "Think ZNC's
perform module, but without the ability to add/remove/rearrange lines
from an IRC query" in sopel-irc#1455.

The commands in the `commands_on_connect` list are executed at the end of
the `startup` procedure. They can also be called by a bot admin with the
`.execute` command.

Note: two `TODO`s were added to adjust docstrings and docs once sopel-irc#1628 is
accepted, since it will change the `ListAttribute` delimiter from
commas to newlines.

Closes sopel-irc#1455
@Exirel Exirel deleted the config-regex-list-attribute branch January 14, 2020 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants