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

Changed QWidget -> QMainWindow to allow the application menu bar. QgsInterface returns a new QToolBar when addToolBar is called #14

Merged
merged 6 commits into from
Jan 18, 2022

Conversation

JaumeFigueras
Copy link
Contributor

Proposal to solve issue #13

@JaumeFigueras
Copy link
Contributor Author

I'm quite new to github. I wanted to create two PR, but my two commits appear in one PR. Also I detected that when a plugin calls the addToolBar function from iface, None was returned. But if this function is called a new toolbar has to be returned to the plugin abling it to add actions.

@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2022

Codecov Report

Merging #14 (f7da8c3) into main (0c13246) will decrease coverage by 0.41%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #14      +/-   ##
==========================================
- Coverage   53.20%   52.79%   -0.42%     
==========================================
  Files           6        6              
  Lines         421      430       +9     
==========================================
+ Hits          224      227       +3     
- Misses        197      203       +6     
Flag Coverage Δ
pytest 52.79% <33.33%> (-0.42%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pytest_qgis/qgis_interface.py 37.11% <30.00%> (+0.03%) ⬆️
src/pytest_qgis/pytest_qgis.py 52.78% <50.00%> (-0.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c13246...f7da8c3. Read the comment docs.

Copy link
Contributor

@Joonalai Joonalai 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 very much for your contribution, it is much appreciated!!

Don't worry about PRs, this is fine if you just modify the title of the PR to address both changes :)

I added some minor comments but other than that, can you just install pre-commit with:

pre-commit install

and that should fix all the code style issues automatically during the next commit. I should add pre-commit instructions to the README.md as well.

src/pytest_qgis/qgis_interface.py Outdated Show resolved Hide resolved
src/pytest_qgis/qgis_interface.py Outdated Show resolved Hide resolved
src/pytest_qgis/qgis_interface.py Outdated Show resolved Hide resolved
JaumeFigueras and others added 2 commits January 18, 2022 09:11
Co-authored-by: Joonalai <33314057+Joonalai@users.noreply.github.com>
Co-authored-by: Joonalai <33314057+Joonalai@users.noreply.github.com>
@JaumeFigueras JaumeFigueras changed the title Changed QWidget -> QMainWindow to allow the application menu bar Changed QWidget -> QMainWindow to allow the application menu bar. QgsInterface returns a new QToolBar when addToolBar is called Jan 18, 2022
@JaumeFigueras
Copy link
Contributor Author

Hello @Joonalai,
I accepted your suggestions, added the pre-commit install and changed the PR title as requested.
Hope everything is OK now.
Thanks for your help!!

Copy link
Contributor

@Joonalai Joonalai left a comment

Choose a reason for hiding this comment

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

Great! Just one more tiny detail :) And I forgot to mention: code style fails, since you did not edit those other files. You have to run pre-commit manually now. Use pre-commit run --all-files in your console.

src/pytest_qgis/qgis_interface.py Outdated Show resolved Hide resolved
@JaumeFigueras
Copy link
Contributor Author

Thank you for your hints.

@Joonalai Joonalai merged commit 0000726 into GispoCoding:main Jan 18, 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.

3 participants