Skip to content

#19 Support Jupyter Notebooks - #182

Open
Iain-S wants to merge 5 commits into
roskakori:mainfrom
Iain-S:notebooks
Open

#19 Support Jupyter Notebooks#182
Iain-S wants to merge 5 commits into
roskakori:mainfrom
Iain-S:notebooks

Conversation

@Iain-S

@Iain-S Iain-S commented Mar 9, 2025

Copy link
Copy Markdown

Summary

Adds support for counting lines of code in Jupyter notebooks, as mentioned in #19.

Changes

  1. Adds JupyterLexer and maps .ipynb files to it.
    • The lexer looks at the notebook metadata to determine which language kernel is used.
    • The lexer needs to see the metadata before it can set self.name so I added the DynamicLexerMixin to indicate a lexer that needs to preview the data.
    • The lexer parses markdown cells using the plain text lexer.
  2. Removes Python 3.8 support as it is end-of-life and also caused a failure in the GitHub action.
  3. Updates dev dependencies section to new Poetry format.

Further Work?

  1. We report the language as Jupyter+Python, which will become Jupyter if merge_embedded_language=True. Is that OK?
  2. I haven't chosen to do anything with raw notebook cells. Would it be better to treat them as documentation?
  3. Would I need to edit the changelog file or anything?

@Iain-S

Iain-S commented Mar 17, 2025

Copy link
Copy Markdown
Author

@roskakori I've merged the recent changes from main if you wanted to take a look / enable the CI workflow now.

@roskakori

Copy link
Copy Markdown
Owner

@Iain-S Thanks for the contribution. As you noticed, I'm just in the process of merging some older pull requests (embarrassingly enough from a sprint performed last summer at the EuroPython 2024 conference 🤫). I hope I get to reviewing yours some time within the next couple of days.

@roskakori roskakori moved this from 🆕 New to 🔖 Ready in Open source projects Mar 18, 2025
@roskakori roskakori added this to the v3.0.0 milestone Mar 18, 2025
@roskakori roskakori changed the title Support Jupyter Notebooks #19 Support Jupyter Notebooks May 2, 2025
@roskakori roskakori self-assigned this May 2, 2025
@roskakori roskakori moved this from 🔖 Ready to 👀 In review in Open source projects May 2, 2025

@roskakori roskakori left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@Iain-S Parsing the notebook as JSON and accessing dictionary keys seems a bit risky to me concerning robustness. Probably we could improve this with some error handling that turn low lever errors like KeyError, JSONDecodeError, or TypeError into something a bit more descriptive.

The concept with peek() and checking for isinstance(..., DynamicLexerMixin) looks a bit clumsy.

I did some research and penned a concept for a Jupyter notebook lexer based on nbformat.reads() and a custom pygments Lexer that seems cleaner and more future-proof to me. See #19 for details.

What are your thought about this? Are you interested in implementing that?

Comment thread pygount/analysis.py


def _delined_tokens(tokens: Iterator[tuple[TokenType, str]]) -> Iterator[TokenType]:
def _delined_tokens(tokens: Iterator[Tuple[TokenType, str]]) -> Iterator[Tuple[TokenType, str]]:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
def _delined_tokens(tokens: Iterator[Tuple[TokenType, str]]) -> Iterator[Tuple[TokenType, str]]:
def _delined_tokens(tokens: Iterator[tuple[TokenType, str]]) -> Iterator[tuple[TokenType, str]]:

The modern, lower case type hints are supported since Python 3.9, which is the lowest version pygount supports.

Comment thread pygount/analysis.py


def _pythonized_comments(tokens: Iterator[tuple[TokenType, str]]) -> Iterator[TokenType]:
def _pythonized_comments(tokens: Iterator[Tuple[TokenType, str]]) -> Iterator[Tuple[TokenType, str]]:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
def _pythonized_comments(tokens: Iterator[Tuple[TokenType, str]]) -> Iterator[Tuple[TokenType, str]]:
def _pythonized_comments(tokens: Iterator[tuple[TokenType, str]]) -> Iterator[tuple[TokenType, str]]:

Again, Python 3.9 syntax.

Comment thread pygount/lexers.py
def peek(self, text) -> None:
"""Look at the text to determine the language."""
self.json_dict = json.loads(text)
self.lexer = pygments.lexers.get_lexer_by_name(self.json_dict["metadata"]["language_info"]["name"])

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Several ways to fail with KeyError, and the lines below, if the dict is incomplete.

Also, if text did not describe a JSON map but for example a string like json.loads('"hello"'), attempting to access it with a key will result in

TypeError: string indices must be integers, not 'str'

@github-project-automation github-project-automation Bot moved this from 👀 In review to 🏗 In progress in Open source projects May 3, 2025
@roskakori roskakori modified the milestones: v3.0.0, v3.1.0 May 23, 2025
@roskakori roskakori moved this from 🏗 In progress to 👀 In review in Open source projects May 23, 2025
@roskakori

Copy link
Copy Markdown
Owner

@Iain-S I think I leave this in limbo for now. Probably we will go for a new PR that implements the concept currently outlines in #19 because it should be more robust.

@roskakori roskakori removed this from the v3.1.0 milestone May 26, 2025
@Iain-S

Iain-S commented May 28, 2025

Copy link
Copy Markdown
Author

@roskakori, by all means leave this in limbo or archive it if you prefer. I would like to think that I would have some time to try the nbformat-based lexer suggestion but I'm not sure when that will be, realistically.

Regarding the

The concept with peek() and checking for isinstance(..., DynamicLexerMixin) looks a bit clumsy.

Yes, it's not ideal. The problem I was trying to work around is that I believe the language needs to be known by the lexer by this line but the lexer won't have seen the source code by that point. Therefore, you would be stuck with a language of "IPython Notebook" or similar. If you want the language of the lexer to reflext the actual language used in the notebook, the lexer needs to be able to inspect the json.

It would probably be useful if you outlined, either here or in the linked issue, where you think language resolution should be done. For example, you call your guess_lexer(), so there could be some custom logic there to allow the notebook lexer to see the source code. Alternatively, you also call l Pygments' get_lexer_for_filename(source_path, text) which shows the lexer the source code but only if there are two or more lexers registered for the same file extension.

So the nbformat lexer may be more robust but I'm not sure it entirely solves the issue. Also, you may (or may not) want to merge in my (or some other) unit tests just so anyone else who tries to implement notebook parsing has some concrete tests to verify against.

@roskakori

Copy link
Copy Markdown
Owner

@Iain-S

It would probably be useful if you outlined, either here or in the linked issue, where you think language resolution should be done.

I added a note to #19 about adding the new lexer to analysis._SUFFIX_TO_FALLBACK_LEXER_MAP.

IIRC lexers in this mapping take priority before guess_lexer() is called, so a plain suffix match suffices.

Also, you may (or may not) want to merge in my (or some other) unit tests just so anyone else who tries to implement notebook parsing has some concrete tests to verify against.

Yes, the tests look good. We should use them no matter the rest of the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 👀 In review

Development

Successfully merging this pull request may close these issues.

2 participants