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

Don't mangle leading hyphens to underscores #2011

Merged
merged 5 commits into from Mar 30, 2021

Conversation

scauligi
Copy link
Member

Fixes #1635.

@Kodiologist
Copy link
Member

The mangling rules are already somewhat complicated, and this makes them thornier. It isn't clear to me in the documentation and tests how leading underscores and leading hyphens are supposed to interact. The order of operations seems to be important.

In test-leading-hyphen, test that the output of mangle equals a constant, rather than testing that it isn't equal to the output of another mangle call.

Did you check for any leading hyphens in the docs that need to be changed?

@peaceamongworlds
Copy link
Contributor

This is more than 3 days old now. Can it get merged?

@Kodiologist
Copy link
Member

Kodiologist commented Mar 25, 2021

No, I've indicated that I don't approve of it in its present form, so the three-day rule doesn't apply. Sunjay needs to either address my concerns or get another reviewer.

@scauligi
Copy link
Member Author

Yup sorry I haven't circled back to this yet, I (unfortunately) have project deadlines that take priority for the time being

@Kodiologist
Copy link
Member

No problem.

@scauligi
Copy link
Member Author

I simplified the leading-hyphen rule to the bare minimum, which makes it much easier to describe in the docs and removes the ambiguity about hyphens vs. underscores:
All non-leading hyphens are converted to underscores, and only the single, initial leading hyphen (if any) is left to be mangled. Thus, --foo-bar-- becomes hyx_XhyphenHminus_foo_bar__; note the underscore before foo which comes from the second hyphen.

I also changed unmangle slightly so it leaves leading and trailing underscores alone, which makes it more style-friendly. E.g., __dundered_name__ unmangles to __dundered-name__ instead of --dundered-name--.

In test-leading-hyphen, test that the output of mangle equals a constant, rather than testing that it isn't equal to the output of another mangle call.

Done!

Did you check for any leading hyphens in the docs that need to be changed?

Yup, (aside from the style guide showing what not to do) I didn't find any leading hyphens in the docs.

@Kodiologist
Copy link
Member

Sorry man, but "It [still] isn't clear to me in the documentation and tests how leading underscores and [a] leading hyphen are supposed to interact. The order of operations seems to be important." Like, what do _-a and -_a mangle to? And what about when a trailing ? creates a leading is_; does that change which hyphens or underscores are considered leading? Try using a more step-by-step approach in the description of mangling in the manual. Handwaving leading underscores as a detail to be described last no longer seems to work well with the new complexity.

Also, your documentation still refers to "leading hyphens" although you're now only treating a single leading hyphen specially.

@Kodiologist
Copy link
Member

Also, I would join your example blocks; e.g.,

::
    => (unmangle 'is_foo_bar)
    "foo-bar?"
::
    => (unmangle 'hyx_XasteriskX)
    "*"
::
    => (unmangle '_hyx_is_fooXsolidusXa)
    "_foo/a?"

can be written

::
    => (unmangle 'is_foo_bar)
    "foo-bar?"
    => (unmangle 'hyx_XasteriskX)
    "*"
    => (unmangle '_hyx_is_fooXsolidusXa)
    "_foo/a?"

@scauligi
Copy link
Member Author

Ok! I've updated the documentation to list the mangling algorithm as a sequence of steps, hopefully it's more clear now how the different mangling rules interact and also clears up that it's just the initial hyphen (if any) that is not converted to an underscore.

Copy link
Member

@Kodiologist Kodiologist left a comment

Choose a reason for hiding this comment

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

Thanks, looks good.

@scauligi
Copy link
Member Author

@peaceamongworlds I've made changes to the new mangling rules since you first approved them, does your approval still stand?

@peaceamongworlds
Copy link
Contributor

@peaceamongworlds I've made changes to the new mangling rules since you first approved them, does your approval still stand?

Yep, I'm still happy with this.

@Kodiologist
Copy link
Member

@peaceamongworlds Thanks, but next time, don't forget to rebase onto the current master before merging.

@peaceamongworlds
Copy link
Contributor

@peaceamongworlds Thanks, but next time, don't forget to rebase onto the current master before merging.

Oh sure. I guess that has to be done manually, since github says that rebase and merge is not enable for this repo?

@Kodiologist
Copy link
Member

It has to be done manually, yes. The "Rebase and merge" option on GitHub is misleading: it rebases all the commits onto master and then fast-forwards master instead of merging. I've disabled this so it isn't done by accident.

@scauligi scauligi deleted the munderscore branch March 31, 2021 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Leading - should not get converted to underscores.
3 participants