Skip to content

feat: provide custom json module#306

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

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

Conversation

@at-cf

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

Copy link
Copy Markdown
Contributor

see #305

clickingbuttons
clickingbuttons reviewed Sep 26, 2022
View reviewed changes

@clickingbuttons clickingbuttons left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread polygon/websocket/__init__.py
clickingbuttons
clickingbuttons approved these changes Oct 4, 2022
View reviewed changes

@clickingbuttons clickingbuttons left a comment

Copy link
Copy Markdown
Contributor

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

clickingbuttons commented Oct 5, 2022

Copy link
Copy Markdown
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

at-cf commented Oct 5, 2022

Copy link
Copy Markdown
Contributor Author

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.

clickingbuttons
clickingbuttons reviewed Oct 5, 2022
View reviewed changes

@clickingbuttons clickingbuttons left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So close...

Comment thread examples/websocket/custom-json.py Outdated
Comment thread examples/rest/custom-json-get.py Outdated
@clickingbuttons

clickingbuttons commented Oct 18, 2022

Copy link
Copy Markdown
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.

clickingbuttons
clickingbuttons approved these changes Oct 28, 2022
View reviewed changes

@clickingbuttons clickingbuttons left a comment

Copy link
Copy Markdown
Contributor

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
@clickingbuttons clickingbuttons mentioned this pull request Oct 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Darcy-Linde Darcy-Linde Awaiting requested review from Darcy-Linde
1 more reviewer
@clickingbuttons clickingbuttons clickingbuttons approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@at-cf @clickingbuttons