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

Fix typographer interpretation of (p) #761

Closed
wants to merge 1 commit into from

Conversation

kemitchell
Copy link

This tiny PR corrects typographer mode's interpretation of (p) and (P).

Currently, those strings get replaced with §, the section symbol. But (p), when used to identify intellectual property rights, like (c) and (tm) and (r), maps to , the phonogram symbol.

Wikipedia has a decent page on the symbol, with references to various treaties and national laws that give it meaning.

Previously, with typographer mode enables, the strings `(p)` and `(P)`
would be rendered as `§`, the section symbol. This patch corrects the
interpretation to `℗`, the sound symbol used to indicate copyrights in
sound recordings.
@puzrin
Copy link
Member

puzrin commented Mar 8, 2021

Sorry, no such breaking changes. This in not fix.

@puzrin puzrin closed this Mar 8, 2021
@kemitchell
Copy link
Author

I understand breakage, but did not know your policy. Perhaps this might better belong behind a configuration flag.

In any event, (P) resolving to the section sign is clearly wrong.

@SamSaffron
Copy link

@puzrin any objections to a PR that makes the list of substitutions configurable, alternative is kind of messy cause we would have to duplicate quite a bit of code?

typographer: { replacementFn: fn } perhaps?

@kemitchell
Copy link
Author

FYI, I have a PR coming for { rare: false, intellectualProperty: false, quotes: true }.

@puzrin
Copy link
Member

puzrin commented Mar 8, 2021

Guys, i argee, current (p) substitution is strange. I don't remember what i drank before decided to add that :). But:

  • Breaking changes are not welcome (without very strong reasons).
  • Change one strange thing to another strange thing is not nice.

Probably, i would agree to just drop that pattern. Need to discuss available alternatives.

@puzrin any objections to a PR that makes the list of substitutions configurable, alternative is kind of messy cause we would have to duplicate quite a bit of code?

typographer: { replacementFn: fn } perhaps?

  • I certainly don't like idea to make options more complex.
  • The first question for any core change request is "why this can not be done via plugins".

If you wish to summarize suggestions - probably worth open new issue.

@puzrin
Copy link
Member

puzrin commented Mar 8, 2021

FYI, I have a PR coming for { rare: false, intellectualProperty: false, quotes: true }.

@kemitchell please, don't PR breaking changes without discussing. All breaking changes are rejected by default. I feel unconmfortable to reject PRs :)

@SamSaffron
Copy link

@puzrin it can be done with a plugin, but I am not a fan of carrying the same code twice in the js payload, probably does not matter that much in the big scheme. Ideally if we just do this by turning off typographer we can build markdown.it without typographer.

@kemitchell
Copy link
Author

Understand the strong compatibility guarantee. But I don't agree this is changing one strange thing to another.

Among lawyers, common digraphs for § include s. and S.. At the same time, (P) is clearly ASCII for ℗, and has been used that way in e-mails and text files for decades. If markdown-it currently rendered (c) as ¶, the pilcrow or paragraph symbol, that would be a clear error, too.

Что касается крепких напитков --- ничего страшнего. )))

@puzrin
Copy link
Member

puzrin commented Mar 8, 2021

@SamSaffron i don't like tendency to clutter api with more and more options. So, everything is directed to external plugins by default. Core code is minimalistic, that's intended.

@SamSaffron
Copy link

understood, but can we build core without typographer?

@puzrin
Copy link
Member

puzrin commented Mar 8, 2021

Among lawyers, common digraphs for § include s. and S.. At the same time, (P) is clearly ASCII for ℗, and has been used that way in e-mails and text files for decades.

At the time i added existing replacement (many years ago), i could not find conflicting alternatives :(

Anyway, making something work completely different is much worse than just make it not work. So,

  • i'm ok to drop (p)
  • i'm not ok to change result.

understood, but can we build core without typographer?

I don't know possibility to do it without ass pain. But we could discuss conflicting things and try to resolve those.

@kemitchell
Copy link
Author

The replacements and smartquotes submodules, which make up the typographer functionality, are hard coded and dynamically loaded in parser_core.js via require. You'd need to fork to drop typographer, but the patch would be small, and might apply cleanly for quite a while, especially with the hard compat policy.

@kemitchell
Copy link
Author

@puzrin if you want it to stay as-is, so be it. Thank you for the work on this module you've done so far. You certainly don't owe me any more work, especially in a direction you don't like.

@puzrin
Copy link
Member

puzrin commented Mar 8, 2021

  • I could drop (p), if it's really an annoying conflict.
  • I don't like to make options more complex.

Note, i reject only concrete implementations, not report of "problem". I agree, conflict exists and existing (p) is strange. If it's important for you - create an issue with your thoughts.

@kemitchell
Copy link
Author

@puzrin I'm not so interested in this pull request that I want to discuss it at length in an issue, as well.

I wanted to make you aware of the problem, and I see that you are aware. Thanks for your attention.

@puzrin
Copy link
Member

puzrin commented Mar 8, 2021

#763

Added memo (without deadline). If you need to enforce priority - feel free to write your thoughts.

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.

None yet

3 participants