Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR: Rename QtDatavisualization to use uppercase v #228

Merged
merged 2 commits into from
Aug 24, 2021

Conversation

antlarr
Copy link
Contributor

@antlarr antlarr commented Jan 14, 2021

Both PyQt5 and PySide2 call the module QtDatavisualization (with an uppercase v) and the tests/test_qtdatavisualization.py file even does pytest.importorskip("qtpy.QtDataVisualization") which means the test is currently always skipped since the module didn't exist.

Another solution would be to fix the test but for homogeneity with PyQt5 and PySide2 I think it's better to rename QtDatavisualization.py to QtDataVisualization.py .

Both `PyQt5` and `PySide2` call the module QtDatavisualization (with an
uppercase V) and the `tests/test_qtdatavisualization.py` file even
does `pytest.importorskip("qtpy.QtDataVisualization")` which means the
test is always skipped since the module didn't exist.

Another solution would be to fix the test but for homogeneity with
PyQt5 and PySide2 I think it's better to rename QtDatavisualization.py
to QtDataVisualization.py .
@bnavigator
Copy link

Should QtPy provide an alias set in __init__.py for backwards compatibility?

There might be applications using the wrong name. This
commit introduces an alias so they keep working as usual.
Copy link
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.

Makes sense to me, esp. with the backward compat. However, I am kinda unclear on the PR description; you say

Both PyQt5 and PySide2 call the module QtDatavisualization (with an uppercase v)

but while you say they call the module "with an uppercase v", the actual text written there as to what they call the module has a lowercase v, so I'm a bit confused. Other than that, LGTM FWIW once we get the CIs running again (not sure what happened with them).

@antlarr
Copy link
Contributor Author

antlarr commented Mar 19, 2021

Oops, that's a very unfortunate typo. I should have written QtDataVisualization indeed.

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

Thanks; FWIW, LGTM. You might need to repush (e.g. git commit --amend --no-edit && git push -f) to trigger the checks once PR #208 is merged, which should hopefully happen shortly.

@ccordoba12 ccordoba12 changed the title Rename QtDatavisualization.py to use uppercase v PR: Rename QtDatavisualization to use uppercase v Aug 23, 2021
@ccordoba12 ccordoba12 added this to the v1.11.0 milestone Aug 23, 2021
@dalthviz
Copy link
Member

Closing (and reopening) to check if a CI rerun is triggered

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

Thank you @antlarr ! LGTM 👍

@dalthviz dalthviz merged commit aa76889 into spyder-ide:master Aug 24, 2021
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.

5 participants