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

101 webconnections not being added #115

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

ceftanveer
Copy link
Collaborator

Web connections added via pyqgis api
This removes dependency from the Qgis versions.

@ceftanveer ceftanveer requested a review from cefect January 26, 2025 01:02
@ceftanveer ceftanveer linked an issue Jan 26, 2025 that may be closed by this pull request
@cefect cefect changed the base branch from master to dev January 26, 2025 01:03
canflood/misc/webConnections.py Outdated Show resolved Hide resolved
@ceftanveer ceftanveer requested a review from cefect January 27, 2025 00:05
@ceftanveer
Copy link
Collaborator Author

@cefect Updated with relative paths and added web connections. Need to verify the connections of all the servers. Will look into that further. However connections are getting added.
image

canflood/misc/webConnections.py Outdated Show resolved Hide resolved
@ceftanveer ceftanveer requested a review from cefect January 31, 2025 01:50
@ceftanveer
Copy link
Collaborator Author

@cefect Made updates to PR, adding different connections WMS, WCS and arcgis REST. All getting added. Looking into correcting links for GRA15 and NPRI

Copy link
Collaborator

@cefect cefect left a comment

Choose a reason for hiding this comment

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

see 1 commnet. also, need to add a test

canflood/misc/webConnections.py Outdated Show resolved Hide resolved
@ceftanveer ceftanveer requested a review from cefect February 1, 2025 06:17
from misc.webConnections import WebConnect

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cefect Updated test code to ensure, mock user ini file is update and then only test passes.

@ceftanveer
Copy link
Collaborator Author

@cefect PR is updated, Test code is updated, and is ready for review

Copy link
Collaborator

@cefect cefect left a comment

Choose a reason for hiding this comment

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

see comments. also, are all the tests passing? if not, need to include explanation for each failure.

tests2/Misct/test_webconnections.py Outdated Show resolved Hide resolved
)
def test_read_connections(web_connect, config_file, expected_key):
"""Test the read_connections function and check expected QGIS3.ini structure."""
config_path, expected_key = config_file
Copy link
Collaborator

Choose a reason for hiding this comment

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

use explicit parameters. i.e., I expect to see 'config_path' in the pytest parameters if this is the parameter you are using. i.e., don't use a nested dictionary like this.

Also, what is going on with 'expected_key' here? It looks like you define it twice?

Also, why not just use the uri info in .\canflood\_pars\WebConnections.ini? That would save you having to write out all these url strings AND provide a test on the .ini file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Refactored the code and used explicit parameters. and removed the redundancy of expected_key.

@cefect just tried to keep the test separate from the script. Do you suggest using already present WebConnections.ini ?

tests2/Misct/test_webconnections.py Outdated Show resolved Hide resolved
canflood/misc/webConnections.py Outdated Show resolved Hide resolved
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.

WebConnections not being added
3 participants