Skip to content

PR: toPython helpers for QDate and QTime#361

Merged
dalthviz merged 4 commits intospyder-ide:masterfrom
bob-schumaker:topython-additions
Aug 12, 2022
Merged

PR: toPython helpers for QDate and QTime#361
dalthviz merged 4 commits intospyder-ide:masterfrom
bob-schumaker:topython-additions

Conversation

@bob-schumaker
Copy link
Copy Markdown
Contributor

@bob-schumaker bob-schumaker commented Aug 11, 2022

This adds toPython() compat functions to QDate and QTime for symmetry.

@CAM-Gerlach CAM-Gerlach changed the title toPython helpers for QDate and QTime PR: toPython helpers for QDate and QTime Aug 12, 2022
CAM-Gerlach
CAM-Gerlach reviewed Aug 12, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Seems naively reasonable to me overall, but I'd defer to @dalthviz on that.

Could you either add tests like the one for QDateTime.toPython (or parameterize that test accordingly)?

@CAM-Gerlach CAM-Gerlach requested a review from dalthviz August 12, 2022 00:18
@bob-schumaker @CAM-Gerlach
Update qtpy/QtCore.py
503a07d 
Missed a step that didn't get caught in testing for some reason.

Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
dalthviz
dalthviz reviewed Aug 12, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Hi @bob-schumaker thank you for working on this! I would say that the missing thing here is to add some tests for QDate.toPython and QTime.toPython as @CAM-Gerlach mentioned. If you need help with that let us know!

Other than that this LGTM 👍

@dalthviz dalthviz added this to the v2.2.1 milestone Aug 12, 2022
@bob-schumaker
Copy link
Copy Markdown
Contributor Author

bob-schumaker commented Aug 12, 2022

Hi @bob-schumaker thank you for working on this! I would say that the missing thing here is to add some tests for QDate.toPython and QTime.toPython as @CAM-Gerlach mentioned. If you need help with that let us know!

Other than that this LGTM 👍

Duh, out of practice. Let me add the tests :-)

dalthviz
dalthviz approved these changes Aug 12, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @bob-schumaker !

@dalthviz dalthviz merged commit 75984bc into spyder-ide:master Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CAM-Gerlach CAM-Gerlach CAM-Gerlach left review comments

@dalthviz dalthviz dalthviz approved these changes

Assignees

@bob-schumaker bob-schumaker

Labels

type:Enhancement

Projects

None yet

Milestone

v2.2.1

Development

Successfully merging this pull request may close these issues.

3 participants

@bob-schumaker @dalthviz @CAM-Gerlach