Skip to content

WIP record parser#2

Merged
gaborbernat merged 15 commits intopypa:masterfrom
uranusjr:record-parser
May 1, 2020
Merged

WIP record parser#2
gaborbernat merged 15 commits intopypa:masterfrom
uranusjr:record-parser

Conversation

@uranusjr
Copy link
Copy Markdown
Member

@uranusjr uranusjr commented Apr 30, 2020

Let’s get something started? :p

First posted to make sure I’m on the right track. I’ll rewrite this to be Python 2.7 compatible if this approach looks valid. Already did.

@uranusjr uranusjr marked this pull request as ready for review May 1, 2020 06:00
Copy link
Copy Markdown

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

I would personally not put the record parsing within the wheels module, but rather a package 👍

@uranusjr
Copy link
Copy Markdown
Member Author

uranusjr commented May 1, 2020

Any suggestion on the module/package name? I need somewhere to put the parser, and wheels.py is rather arbitrary. Another idea is records.py which can also contain the RECORD writer.

@gaborbernat
Copy link
Copy Markdown

I agree on records.py that's part of a package installer. Generally, the installer will only understand wheels so I don't think it makes sense to add a wheels package.

@uranusjr
Copy link
Copy Markdown
Member Author

uranusjr commented May 1, 2020

Heh, isort and black are refusing to work together.

@brettcannon
Copy link
Copy Markdown
Member

@uranusjr we get semi-regular issues from isort and Black users on the Python extension for VS Code from complaining about them competing with each other. 😆

@gaborbernat
Copy link
Copy Markdown

Did we set up the configuration to match the style? If so should be just one issue (when there's a comment for an import)?

Comment on lines +3 to +6
try: # pragma: no cover
from typing import TYPE_CHECKING
except ImportError: # pragma: no cover
TYPE_CHECKING = False
Copy link
Copy Markdown
Member

@pradyunsg pradyunsg May 1, 2020

Choose a reason for hiding this comment

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

Suggested change
try: # pragma: no cover
from typing import TYPE_CHECKING
except ImportError: # pragma: no cover
TYPE_CHECKING = False
if False:
from typing import TYPE_CHECKING
else:
TYPE_CHECKING = False # type: ignore

nit: let's not import typing even on Python 2 (to avoid uninstalling-in-use-module issues if pip vendors this)?

Copy link
Copy Markdown
Member Author

@uranusjr uranusjr May 1, 2020

Choose a reason for hiding this comment

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

In that case, why not just TYPE_CHECKING = False?

Copy link
Copy Markdown
Member

@pradyunsg pradyunsg May 4, 2020

Choose a reason for hiding this comment

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

To make mypy/typing/pytype actually treat this value as "ALWAYS FALSE", which is different from a False assignment.

@gaborbernat gaborbernat merged commit 9b2c0ae into pypa:master May 1, 2020
@pradyunsg
Copy link
Copy Markdown
Member

pradyunsg commented May 4, 2020

:/

Don't merge PRs with a WIP in the heading? I never got the chance to respond to comments I made, or review this PR.

@gaborbernat
Copy link
Copy Markdown

I thought draft PRs are for that 🤔

@pradyunsg
Copy link
Copy Markdown
Member

I agree that draft PRs also indicate this in a much better way. However, the title clearly says "WIP" (it still does) which is a fairly non-ambiguously indicates the same thing.

If y'all reckon PRs are OK to merge while "WIP" is the first thing in their titles, I disagree with that. Anyway, it was very surprising that a PR with WIP in it's title got merged, and I was certainly caught off guard by that.

I don't really have much to say other than, well, I was caught off guard.

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.

4 participants