Skip to content

Comments

feat: provide custom json module#306

Merged
clickingbuttons merged 8 commits intomassive-com:masterfrom
at-cf:master
Oct 28, 2022
Merged

feat: provide custom json module#306
clickingbuttons merged 8 commits intomassive-com:masterfrom
at-cf:master

Conversation

@at-cf
Copy link
Contributor

@at-cf at-cf commented Sep 26, 2022

see #305

Copy link
Contributor

@clickingbuttons clickingbuttons left a comment

Choose a reason for hiding this comment

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

Thanks for contributing. Code looks good! Just need to add :param custom_json: Your explanation to the doc comments and a better type than Any. Bonus points for an example.

Copy link
Contributor

@clickingbuttons clickingbuttons left a comment

Choose a reason for hiding this comment

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

Looks awesome! Thanks for contributing :)

@clickingbuttons
Copy link
Contributor

Once you fix the tests and lint this should be good. Run make lint in the root to fix the lint. For the tests make sure in the test environment the Any type is defined. It might need to be imported from somewhere.

@at-cf
Copy link
Contributor Author

at-cf commented Oct 5, 2022

Once you fix the tests and lint this should be good. Run make lint in the root to fix the lint. For the tests make sure in the test environment the Any type is defined. It might need to be imported from somewhere.

Sorry, I am on win64 and it looks like you guys are set up for unix, so I can't easily use your scripts. I committed some fixes.

Copy link
Contributor

@clickingbuttons clickingbuttons left a comment

Choose a reason for hiding this comment

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

So close...

@clickingbuttons
Copy link
Contributor

Hey @at-cf. It looks like this PR just needs some minor linting fixes. If you don't respond or don't have time I'll resolve them later this week and merge.

Copy link
Contributor

@clickingbuttons clickingbuttons left a comment

Choose a reason for hiding this comment

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

I fixed this up and it's good to merge. I just had to poetry add -D orjson and I was able to import from it directly. Since the package doesn't have types I had to add # type: ignore before it.

@clickingbuttons clickingbuttons merged commit 8c54311 into massive-com:master Oct 28, 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.

2 participants