Skip to content

Conversation

@meshy
Copy link
Collaborator

@meshy meshy commented Jan 3, 2024

This is mostly just moving code around, but does add a "SUCCESS" return-code, and do a couple of renames.

I intend to add more commands (as a re-work of #18), and getting this out of the way will make that simpler.

Before this the `mypy_json_report` module was a single file.

By using a `mypy_json_report` package we'll be able to split logic up
into multiple files, and maintain the current namespace.
@meshy meshy marked this pull request as ready for review January 3, 2024 16:45
@meshy meshy requested a review from Tenzer January 3, 2024 16:59
Copy link
Member

@Tenzer Tenzer left a comment

Choose a reason for hiding this comment

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

Looks good.

You should change the entrypoint for the package so it points at the new location for the main function:

mypy-json-report = "mypy_json_report:main"

meshy added 12 commits January 4, 2024 09:57
This is consistent with the name of the class.
This will make working with these types easier, and better allows for
future expansion.
Previously, we composed our parsing objects in the same function
as we executed the parsing logic.

This change separates those concerns, and will make splitting up this
file easier.
These tests were only concerned with the parse command, so this name is
more fitting.
While these are not (currently) explicitly used, it's nice to have them
here for reference.
@meshy meshy force-pushed the introduce-modules branch from 3ec3a5c to 0ef89af Compare January 4, 2024 10:52
@meshy
Copy link
Collaborator Author

meshy commented Jan 4, 2024

Thanks @Tenzer. I think I've addressed your points now.

Copy link
Member

@Tenzer Tenzer left a comment

Choose a reason for hiding this comment

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

Thanks!

@Tenzer Tenzer merged commit 8247df0 into main Jan 5, 2024
@Tenzer Tenzer deleted the introduce-modules branch January 5, 2024 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants