SPlanzer commented 4 years ago

Fixes: #

31

26

Change Description:

Ports plugin form QGIS2 to QGIS3 ...

Notes for Testing:

...

Source Code Documentation Tasks:

[ ] README updated (where applicable)
[ ] CHANGELOG updated

User Documentation Tasks:

[ ] Confluence user guide updated (where applicable)

Testing Tasks:

[ ] Added tests that fail without this change
[ ] All tests are passing in development environment
[ ] Reviewers assigned

Pull Request Management:

[ ] Linked to sub-task
[ ] Linked to the epic

SPlanzer commented 4 years ago

Still work to do to complete this porting and therefore PR

palmerj commented 4 years ago

Still work to do to complete this porting and therefore PR

Nice work Simon. Great to see this really moving. Are you going to add some basic CI tests before merging this?

SPlanzer commented 4 years ago

Nice work Simon. Great to see this really moving. Are you going to add some basic CI tests before merging this?

Tests are to be written retrospectively post porting and therefore will not be included in this pull requests.

formatting and linting are in scope.

palmerj commented 4 years ago

Tests are to be written retrospectively post porting and therefore will not be included in this pull requests.

Not ideal. I wonder if basic tests should be at least added to e.g load the plugin, load dialogues etc

formatting and linting are in scope.

At least pyLinting will ensure that the plain python code is likely to compile. But won't check the QGIS and QT APIs

billgeo commented 4 years ago

I wonder if basic tests should be at least added to e.g load the plugin, load dialogues etc

I agree with this. Even if it's just the load the plugin bit. Could you put in some simple tests @SPlanzer?

SPlanzer commented 4 years ago

I agree with this. Even if it's just the load the plugin bit. Could you put in some simple tests @SPlanzer?

okay

strk commented 4 years ago

Now that the plugin-ci branch was merged there are docker images and a docker-compose to start containers from them in order to test the plugin. NO automated plugin loading test exists as of yet, but it's very easy to do a manual test (just run make docker-qgis-start from top-level dir).

Note that such manual test currently is able to reproduce the issue reported in #82 (issue is NOT at "plugin load time" but later in the process [when clicking the activation button])

About this PR: is the idea to keep the plugin compatible with both QGIS2 and QGIS3 or are we going to keep 2 separate branches for the 2 versions ? I'll work on #83 next, to provide a QGIS3 docker image as well, but if we're going to use separate branches it may be a good idea to have either one or the other in a single branch, not both ?

strk commented 4 years ago

@SPlanzer while working on the QGIS3 docker image (which I'm basing on this PR), I found that the LINZ/gazetteer/gui/DatabaseConfiguration.py script fails to run because it uses /usr/bin/python, which points to python2.

Now.. should we change all those scripts to explicitly request python3 instead ? (with something like #!/usr/bin/env python3)

strk commented 4 years ago

Note I fixed the python3 issue with code in #86, which can be merged into this PR

strk commented 4 years ago

@SPlanzer ping me when you need a new review (make sure to solve conflicts first) - Thank you !

SPlanzer commented 4 years ago

@SPlanzer ping me when you need a new review (make sure to solve conflicts first) - Thank you !

@strk Please review. This is in a state where it is running in QGIS3 without any major bugs (I am sure there are still some but these will be picked up with automated tests)

I have turned off commit message git-liniting just for this pull request as there are many commits not following it.
- I will reactivate this in a PR once this has been merged https://github.com/linz/gazetteer/blob/4603d9d2b0bc4e2b2147db6f6ef90571b30c586a/.github/workflows/push.yml#L28-L30
White space checking added to on push but commented out

https://github.com/linz/gazetteer/blob/4603d9d2b0bc4e2b2147db6f6ef90571b30c586a/.github/workflows/push.yml#L21-L26

It will be messy to fix all formatting in this pull request. As you have pointed out this should have been done prior to any code edits. I will submit a pull request once this one has been merged to resolve all formatting issues.

strk commented 4 years ago

Note that you reverted, with 1dd5c4fa86a4873f87b524920a24afdbe5b4e24b, a change I made to the import line to make Config found (9fd034ecccbcf6f140897db768ba593539ca38b4).

SPlanzer commented 4 years ago

Beside the import Config changed back to from . import Config, which breaks running LINZ/gazetteer/gui/DatabaseConfiguration.py as done by .docker/qgis/scripts/docker-entry.sh,

I have been doing my testing on a local install of QGIS3 rather than the docker image. import Config throws the 'No module named 'Config'' and from . import Config fixes this. This is inline with my understanding of py3 relative imports where a single dot means that the module or package referenced is in the same directory as the current location

this commit contains a lot of stylistic-only changes. Were they introduced due to some editor setup you have to enforce a given style ?

Yeah, we are using Black for python projects across LI. As part of trying to configure eclipse to do .editorconfig checking (which I still have not solved) I configured automated black formatting.

I understand this is messy to include format changes with functionality fixes and in hindsight formatting should have been resolved first. Once this is merged I will undertake all formatting fixes.

strk commented 4 years ago

Should this ticket be attached to an Epic or did the Roadmap thing change policy ? Sorry if I'm confused by overtooling :) \cc @billgeo

strk commented 4 years ago

Found and linked

billgeo commented 4 years ago

Sorry for any confusion, but in answer to your questions, there's no change to how we manage issues and epics, roadmap will be updated once 4 times per year and then after that we will manages all work as epics and sprints.

Should this ticket be attached to an Epic

Yes

or did the Roadmap thing change policy ?

No

strk commented 4 years ago

@SPlanzer pip install -r requirements-dev.txt fails with:

Collecting pre-commit==2.1.1 (from -r requirements-dev.txt (line 2))
  Cache entry deserialization failed, entry ignored
  Could not find a version that satisfies the requirement pre-commit==2.1.1 (from -r requirements-dev.txt (line 2)) (from versions: 0.2.0, 0.2.1, 0.2.2, 0.2.3, 0.2.4, 0.2.5, 0.2.6, 0.2.7, 0.2.8, 0.2.9, 0.2.10, 0.2.11, 0.3.0, 0.3.1, 0.3.2, 0.3.3, 0.3.4, 0.3.5, 0.3.6, 0.4.0, 0.4.1, 0.4.2, 0.4.3, 0.4.4, 0.5.0, 0.5.1, 0.5.2, 0.5.3, 0.5.4, 0.5.5, 0.6.0, 0.6.1, 0.6.2, 0.6.3, 0.6.4, 0.6.5, 0.6.6, 0.6.7, 0.6.8, 0.7.0, 0.7.1, 0.7.2, 0.7.3, 0.7.4, 0.7.5, 0.7.6, 0.8.0, 0.8.1, 0.8.2, 0.9.0, 0.9.1, 0.9.2, 0.9.3, 0.9.4, 0.10.0, 0.10.1, 0.11.0, 0.12.0, 0.12.1, 0.12.2, 0.13.0, 0.13.1, 0.13.2, 0.13.3, 0.13.4, 0.13.5, 0.13.6, 0.14.0, 0.14.1, 0.14.2, 0.14.3, 0.15.0, 0.15.1, 0.15.2, 0.15.3, 0.15.4, 0.16.0, 0.16.1, 0.16.2, 0.16.3, 0.17.0, 0.18.0, 0.18.1, 0.18.2, 0.18.3, 1.0.0, 1.0.1, 1.1.0, 1.1.1, 1.1.2, 1.2.0, 1.3.0, 1.4.0, 1.4.1, 1.4.2, 1.4.3, 1.4.4, 1.4.5, 1.5.0, 1.5.1, 1.6.0, 1.7.0, 1.8.0, 1.8.1, 1.8.2, 1.9.0, 1.10.0, 1.10.1, 1.10.2, 1.10.3, 1.10.4, 1.10.5, 1.11.0, 1.11.1, 1.11.2, 1.12.0, 1.13.0, 1.14.0, 1.14.1, 1.14.2, 1.14.3, 1.14.4, 1.15.0, 1.15.1, 1.15.2, 1.16.0, 1.16.1, 1.17.0, 1.18.0, 1.18.1, 1.18.2, 1.18.3, 1.19.0, 1.20.0, 1.21.0)
No matching distribution found for pre-commit==2.1.1 (from -r requirements-dev.txt (line 2))

Note I was looking at it because ipython is also needed, for docker-compose, or I did get, on docker-compose up:

ImportError: No module named shutil_get_terminal_size

strk commented 4 years ago

Re docker-compose: it was (as usual) a pip related issue, installing via apt fixed the docker-compose part.

Now make docker-qgis-start correctly starts docker containers and runs qgis with GUI connected to desktop, but upon QGIS start there's a python error window saying:

An error occurred during execution of following code:
spec.loader.exec_module(module)

Traceback (most recent call last):
  File "", line 1, in 
  File "", line 724, in exec_module
  File "", line 860, in get_code
  File "", line 791, in source_to_code
  File "", line 219, in _call_with_frames_removed
  File "/root/.local/share/QGIS/QGIS3/startup.py", line 21
    os.environ["GAZETTEER_DBHOST"] = ${PGHOST}
                                     ^
SyntaxError: invalid syntax

Sounds like ${PGHOST} should really be os.environ['PGHOST']

SPlanzer commented 4 years ago

Sounds like ${PGHOST} should really be os.environ['PGHOST']

Its not required. I shoul dnot have added this in the first place. The envi vars are set by docker.

SPlanzer commented 4 years ago

As of 8d97ea0 both make docker-qgis-start and make docker-qgis-test work fine for me. Happy for this to be merged. Possibly with some cleanups, like removing commits that are not useful (for example e7383a6) and squashing commits that do and undo things, like f77d479 and e05bcf7

agreed

strk commented 4 years ago

QGIS issue created: https://github.com/qgis/QGIS/issues/36187

strk commented 4 years ago

Kudos ! Can the branch be deleted now ? (please do!)

linz / gazetteer

2to3 #85

31

26

Change Description:

Notes for Testing:

Source Code Documentation Tasks:

User Documentation Tasks:

Testing Tasks:

Pull Request Management: