Skip to content

Undocumented syntax {} = {:} since v2.5.0 #1711

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

Closed
jayvdb opened this issue Oct 23, 2020 · 3 comments · Fixed by #1715
Closed

Undocumented syntax {} = {:} since v2.5.0 #1711

jayvdb opened this issue Oct 23, 2020 · 3 comments · Fixed by #1715
Labels
bug:normal affects many people or has quite an impact

Comments

@jayvdb
Copy link

jayvdb commented Oct 23, 2020

Prior to v2.5, {} was invalid. Since v2.5, it has become {:} to resolve to os.pathsep, because 9463f8f placed the logic before the following check for "no substitution type", rendering it dead code

        try:
            sub_type = g['sub_type']
        except KeyError:
            raise tox.exception.ConfigError(
                "Malformed substitution; no substitution type provided")

Given how long {} has been valid, perhaps it should be documented as-is, and tests added for {} being os.pathsep. Maybe noted as deprecated and will become invalid in v4.

However even if {} is kept as-is, the above ConfigError is unreachable code, so it should be removed.

I would prefer that we reject {} in v3 with a specific error message that {:} should be used instead. Rejecting {} simplifies/tighens the parsing logic, especially when approaching escaping { and } for #1708 .

@jayvdb jayvdb added the bug:normal affects many people or has quite an impact label Oct 23, 2020
@jayvdb
Copy link
Author

jayvdb commented Oct 23, 2020

I would prefer that we reject {} in v3 with a specific error message that {:} should be used instead.

@gaborbernat @asottile @obestwalter , could you comment on whether this is acceptable in v3. It is likely there are a few users which have used this undocumented {}, but it is easy for them to use {:} instead. Having two ways to achieve the same thing is bad, and {} being {:} is not intuitive.

Rejecting {} allows me to finish other PRs with less effort, and cleaner code.

@gaborbernat
Copy link
Member

Rejecting {} allows me to finish other PRs with less effort, and cleaner code.

👍

jayvdb added a commit to jayvdb/tox that referenced this issue Oct 24, 2020
@jayvdb
Copy link
Author

jayvdb commented Oct 24, 2020

There is also a related parsing problem, that I'll fix at the same time.
{:abc} on master causes

ConfigError: substitution key '' not found

g = {'default_value': 'abc', 'sub_type': None, 'substitution_value': ''} falls through to the bottom of _replace_match, invoking _replace_substitution, which then calls _substitute_from_other_section with an empty key.

ftr, the try/except block dates back to 2f98ebb (pre v1.0), but it appears to have taken a turn for the worse at 0068c4d (v1.2), likely resulting varying outcomes as other parts of the logic changed over time. The fact this hasnt been reported as a bug suggests the syntax has generally been intuitive enough, and/or users are not bothered by odd results and fix their syntax rather than rasing a bug.

jayvdb added a commit to jayvdb/tox that referenced this issue Oct 24, 2020
jayvdb added a commit to jayvdb/tox that referenced this issue Oct 24, 2020
gaborbernat pushed a commit that referenced this issue Oct 24, 2020
@tox-dev tox-dev locked and limited conversation to collaborators Jan 14, 2021
ssbarnea pushed a commit to ssbarnea/tox that referenced this issue Apr 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug:normal affects many people or has quite an impact
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants