distutils2: extension section uses bad environment marker separator

[elicollins]

BPO	12424
Nosy	@tarekziade, @merwok
Files	cpython_issue12424.patch 9170231ebf14.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/merwok'
closed_at = <Date 2014-03-13.02:38:43.093>
created_at = <Date 2011-06-28.00:12:46.262>
labels = ['type-bug']
title = 'distutils2: extension section uses bad environment marker separator'
updated_at = <Date 2014-03-13.02:38:43.092>
user = 'https://bugs.python.org/elicollins'

bugs.python.org fields:

activity = <Date 2014-03-13.02:38:43.092>
actor = 'eric.araujo'
assignee = 'eric.araujo'
closed = True
closed_date = <Date 2014-03-13.02:38:43.093>
closer = 'eric.araujo'
components = ['Distutils2']
creation = <Date 2011-06-28.00:12:46.262>
creator = 'eli.collins'
dependencies = []
files = ['22503', '23247']
hgrepos = ['70']
issue_num = 12424
keywords = ['patch']
message_count = 6.0
messages = ['139339', '144557', '148118', '148351', '148406', '148408']
nosy_count = 4.0
nosy_names = ['tarek', 'eric.araujo', 'alexis', 'eli.collins']
pr_nums = []
priority = 'normal'
resolution = 'out of date'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue12424'
versions = ['Python 3.3']

[elicollins]

The _pop_values() function in packaging.config uses "--" as the environment marker separator when parsing extension sections. This doesn't match PEP-345, or how the metadata section is currently parsed, both of which use ";" instead. Also, "--" is frequently found in compiler option strings, which will probably cause conflicts in the future.

The attached patch changes _pop_values() and related unit tests to use ";" as the separator, as well as tries to clarify some of _pop_values()'s internal comments.

[elicollins]

I've attached a second revision of my patch to fix this issue (9170231ebf14.diff).

Per Eric Araujo's advice, this patch changes _pop_values() to use ";;" as the environment marker separator for all fields in setupcfg's extension sections. I've also updated the documentation and related unittests.

[merwok]

Does your latest patch address my second review?

[elicollins]

The second patchset (9170231ebf14.diff) should implement all the changes you suggested in your second review (dated 2011-09-05).

---

The only non-addressed item in your second review was a request for clarification on a potential error I noticed (and described somewhat poorly). On later examination, I realized I was mistaken, so didn't want to trouble anyone further with it.

(If you want details... I noticed that if a line ends with ";;", _pop_values() will call interpret() with an empty string. I initially though that would cause an error, but later realized it just means interpret() would return False, causing the line to be ignored, which is probably fine for that border case - especially since packaging.metadata has the same behavior for lines ending with ";").

[merwok]

if a line ends with ";;", _pop_values() will call interpret() with an empty string. [...]
it just means interpret() would return False, causing the line to be ignored, which is probably
fine for that border case

Hm, I’d rather call that a bug. _pop_values should not call interpret, or interpret('') should return True (there is no condition that fails), or _pop_values should raise an exception (pro: errors should never pass silently, con: it’s not an error if we define that the empty string is a no-op, and it’s more user-friendly to do that). I’m inclined to let interpret(''), do you think it’s sensible?

[merwok]

Latest patch looks good.

Looking at the examples with ';;' or even '--', I think that their role is non-obvious. Even though the PEP and docs explained them, there is value in using intuitive (“guessable”) markup. So if the constraints are that a marker delimiter should be non-ambiguous and short, why not using something like '#IF#' or '#condition:'? (This will probably require agreement on the fellowship mailing list.)