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

Register new style #2183

Merged
merged 4 commits into from
Jul 26, 2022
Merged

Register new style #2183

merged 4 commits into from
Jul 26, 2022

Conversation

flywire
Copy link
Contributor

@flywire flywire commented Jul 11, 2022

Closes #980

@Anteru
Copy link
Collaborator

Anteru commented Jul 11, 2022

Could you please add a test which exercises the new behavior?

@jeanas
Copy link
Contributor

jeanas commented Jul 11, 2022

Could you please add a test which exercises the new behavior?

Um, for a doc PR about patching the sources?

Though, isn't it better to document how to register a style via a plugin rather than by patching your Pygments distribution?

@Anteru
Copy link
Collaborator

Anteru commented Jul 11, 2022

Ok so wait, default_style was never used previously? I thought this would be an observable behavior change if someone relied on default_style but guess I was mistaken?

@jeanas
Copy link
Contributor

jeanas commented Jul 11, 2022

I think you're confusing issues #980, which this fixes, and #2184, which is unrelated.

@birkenfeld
Copy link
Member

Regarding this PR: I think the advice to copy stuff into the Pygments package should be removed wholesale, or at least clarified to only apply if you add a new style for submission as a PR.

@jeanas
Copy link
Contributor

jeanas commented Jul 11, 2022

Regarding this PR: I think the advice to copy stuff into the Pygments package should be removed wholesale, or at least clarified to only apply if you add a new style for submission as a PR.

+1

@flywire
Copy link
Contributor Author

flywire commented Jul 12, 2022

@birkenfeld bd9eff0 only tries to represent the existing information more simply. The main need for it is the manual change to __init__.py. It would be easier if you just made any change you have in mind.


https://pygments.org/docs/ Hacking for Pygments is written around changing the Pygments package. I don't disagree it should be written around developing a Plugin but the section would need rewriting. It should start with https://pygments.org/docs/plugins/ containing a fully working your plugin consistent with the rest of the section. The emphasis of the other parts of this section should be reworked to be directed more towards the plugin than the package.

@flywire flywire closed this Jul 12, 2022
@flywire flywire reopened this Jul 12, 2022
@jeanas
Copy link
Contributor

jeanas commented Jul 12, 2022

This is from get_style_by_name in styles/__init__.py:

def get_style_by_name(name):
    if name in STYLE_MAP:
        mod, cls = STYLE_MAP[name].split('::')
        builtin = "yes"
    else:
        for found_name, style in find_plugin_styles():
            if name == found_name:
                return style
        # perhaps it got dropped into our styles package
        builtin = ""
        mod = name
        cls = name.title() + "Style"

Ugh. I wonder if this should be left undocumented or just removed.

@flywire
Copy link
Contributor Author

flywire commented Jul 13, 2022

It's not documented in the existing guide but implied. The guide has example style code followed by "That's it.". It truly is it if the file is placed into the style folder and the user doesn't need the services from registering the style (ie bringing up on a list of styles).

It's useful functionality so I wouldn't remove it but it is probably not worth any more documentation than a comment in the code.

@birkenfeld
Copy link
Member

Hm, I don't like that either. It's also different from other components (lexers, formatters).

@Anteru
Copy link
Collaborator

Anteru commented Jul 16, 2022

What's the next step here? It feels like we can merge this change and then continue with the rest elsewhere?

@Anteru Anteru merged commit f162631 into pygments:master Jul 26, 2022
@Anteru
Copy link
Collaborator

Anteru commented Jul 26, 2022

Merged, thanks. I'll add a warning note to the documentation to address @birkenfeld 's feedback. We still need a resolution for get_style_by_name though.

flywire referenced this pull request Jul 26, 2022
Explain that adding styles to the source tree directly is something that
should be only used by contributors.
@Anteru Anteru added this to the 2.13.0 milestone Aug 14, 2022
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.

docs bug - registering new styles
4 participants