Skip to content

Vector init in qt5#1339

Merged
ixjlyons merged 4 commits intopyqtgraph:masterfrom
outofculture:vector-init-qt5
Aug 16, 2020
Merged

Vector init in qt5#1339
ixjlyons merged 4 commits intopyqtgraph:masterfrom
outofculture:vector-init-qt5

Conversation

@outofculture
Copy link
Copy Markdown
Contributor

@outofculture outofculture commented Aug 5, 2020

Vector.__init__ improvements

  • add docstring
  • fix handling of QVector3D args (cannot list() them)
  • refactor to no longer need return statements

Also add Pa to the list of units.

@outofculture outofculture changed the base branch from develop to master August 10, 2020 19:36
* added docstring
* fixed handling of QVector3D args (cannot list() them)
* refactor to no longer need return statements
@outofculture
Copy link
Copy Markdown
Contributor Author

Rebased onto master; turns out the refactor had happened once before, but this one is more accurate.

Copy link
Copy Markdown
Member

@ixjlyons ixjlyons left a comment

Choose a reason for hiding this comment

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

A few nit-pick comments on the docs, but otherwise this looks good to me.


============== ================================================================================================
**Arguments:**
*args* Could be any of:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Vector isn't currently in the docs, but there should be few more spaces introduced before "Could". It should align with the = after the gap in the table borders. Please also add an empty line after this one - I'm not sure exactly why but RST/sphinx seems to prefer it this way for bulleted lists.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks; I was bumbling about for the rules there.

* 1 QSizeF (`0` assumed for z)
* 1 QPointF (`0` assumed for z)
* Any other valid QVector3D init args.
----------------------------------------------------------------------------------------------------------------
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this line can be removed.

============== ================================================================================================
**Arguments:**
*args* Could be any of:
* 3 numerics
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's probably obvious, but maybe add a parenthetical (x, y, and z) at the end of this line.

@outofculture
Copy link
Copy Markdown
Contributor Author

I've acted on all the comments.

@ixjlyons
Copy link
Copy Markdown
Member

Looks good. Thanks @outofculture

@ixjlyons ixjlyons merged commit 11b76a1 into pyqtgraph:master Aug 16, 2020
@outofculture outofculture deleted the vector-init-qt5 branch May 6, 2021 00:38
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