8000 Make Formatter generic for the benefit of typeshed by not-my-profile · Pull Request #2081 · pygments/pygments · GitHub
[go: up one dir, main page]
Skip to content

Make Formatter generic for the benefit of typeshed #2081

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Make Formatter generic for the benefit of typeshed #2081

wants to merge 1 commit into from

Conversation

not-my-profile
Copy link
Contributor

The type stubs for Pygments in the typeshed have made pygments.formatter.Formatter generic in python/typeshed#6819 so that an outfile passed to pygments.highlight() must be opened in binary mode when the formatter was created with a given encoding, for example:

import pygments
import pygments.lexers
import pygments.formatters

lexer = pygments.lexers.get_lexer_by_name('python')
formatter = pygments.formatters.html.HtmlFormatter(encoding='utf-8')

with open('out.html', 'wb') as f: # mypy reports a type error if opened with 'w'
    pygments.highlight('test', lexer, formatter, outfile=f)

This does however come with the disadvantage that subclassing Formatter no longer type checks with mypy --strict:

class Foo(pygments.formatter.Formatter):
     pass

because mypy complains with Missing type parameters for generic type "Formatter". The problem with that is that fixing this by specifying the type parameter would result in a runtime error because the real pygments.formatter.Formatter is not generic and thus not subscriptable.

Making the actual Formatter generic improves the situation for pygments users using static type checkers and should not change anything for the pygments users that don't.

@not-my-profile not-my-profile mentioned this pull request Mar 4, 2022
Closed
@not-my-profile not-my-profile marked this pull request as draft March 4, 2022 11:34
@birkenfeld
Copy link
Member

I'm not very keen on merging this; it would be an isolated case of strange typing stuff without any benefit for us.

If providing types via typeshed cannot handle such cases, it looks like it's buggy and should be fixed.

@not-my-profile
Copy link
Contributor Author

Right, I understand.

@not-my-profile not-my-profile mentioned this pull request Mar 4, 2022
Open
@Dreamsorcerer
Copy link

Providing types via typeshed is already a suboptimal solution and generally considered to be temporary. The design of that class in pygments makes it difficult to type correctly, and impossible to not introduce these discrepancies between stubs and code. Ideally, the typing information should be moved here and supported as part of the project itself.

If that is an option and someone wants to start on it, I'm happy to help with code reviews.

@birkenfeld
Copy link
Member

I cannot say I like how the "optional" typing is creeping in everywhere.

If the other @pygments/maintainers are strongly in favor, I won't veto it, but I won't do the work.

@Anteru
Copy link
Collaborator
Anteru commented Mar 4, 2022 •
edited
Loading

I'd only do typing if we do it a) properly and b) once a newer version of Python which makes it easier to express the information becomes our baseline, most likely 3.10. Till then I see limited value -- and it will leave the codebase in a "half done" state which I really don't look forward to.

@Dreamsorcerer
Copy link

Sounds reasonable. If there are things missing in the typing module from newer versions, you can always just import them from typing_extensions in older python versions, in case you want to do it a bit earlier.

@jeanas
Copy link
Contributor
jeanas commented Mar 4, 2022

I'm totally new as a committer here, but I concur with @birkenfeld and @Anteru. One point I'd like to raise is that, by nature, Pygments is a project attracting many different one-time contributors adding and updating lexers for their pet languages, who are fluent in that language and may not be fluent in Python. Judging from the concepts proposed in typing PEPs posted on python-dev, it seems to me that writing typed Python code still poses a number of challenges, and thus makes for a steeper learning curve for modifying Pygments code than I would prefer.

@danieleades
Copy link
Contributor

I'm totally new as a committer here, but I concur with @birkenfeld and @Anteru. One point I'd like to raise is that, by nature, Pygments is a project attracting many different one-time contributors adding and updating lexers for their pet languages, who are fluent in that language and may not be fluent in Python. Judging from the concepts proposed in typing PEPs posted on python-dev, it seems to me that writing typed Python code still poses a number of challenges, and thus makes for a steeper learning curve for modifying Pygments code than I would prefer.

strong disagree.

making small contributions in an unfamiliar repo is dramatically easier to get right when you have type annotations and the IDE support they enable providing a guardrail

@jeanas jeanas mentioned this pull request Mar 15, 2024
Open
@Akuli Akuli mentioned this pull request Mar 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@not-my-profile @birkenfeld @Dreamsorcerer @Anteru @jeanas @danieleades
0