-
Notifications
You must be signed in to change notification settings - Fork 12
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
Refactoring and testing complete. (+13 squashed commits) #15
base: master
Are you sure you want to change the base?
Conversation
Squashed commits: [abeb938] Should have all Symbol Sets correctly coded and User experience with the drop downs fully coded and bug free. (fingers crossed.) [b5b472d] more refactoring [0a7e0ac] more refactoring [fde6ed3] more refactoring [6c8dcf9] more refactoring [a4ddae0] more refactoring [8e059cd] more refactoring [4b27b14] continuing edits to LandUnitSymbolSet [ec937bc] continuing edits to LandUnitSymbolSet [11afe22] building out the land unit symbol set. [7ac134b] building out the land unit symbol set. [3d788a1] refactoring, creating individual classes for the respective symbol sets. Have updated UI to reflect the workflow of https://explorer.milsymb.net/#/explore/. [4a85413] Make SIDC dialog run-able; use old-style ui import for code completion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial testing...
-
The
SymbolSets.py
file should follow Pythonic naming, e.g.symbolsets.py
. The file is missing a copyright header. Each class in the file should inherit from a base class, maybeSymbolSet
, instead of reassigning all of the instance variables in__init__()
, which I don't think is doing anything now. -
When testing, I noticed the renderer showed correct symbol, but not the edit dialog (
Entity
is not set to11 Military
for code10010535221100000000
):
-
Then, after setting
Entity
to11 Military
, and then trying to set it back to00 Not Applicable
, this error is thrown:
-
The edit dialog has been changed, so the Windows screen snap in the docs need updated, as well as any associated text.
-
Please update the copyright headers for files you have worked on, e.g. add this year, etc.
-
Most importantly, there are no unit tests for the changes. While previously, the
tester
plugin was used functionally, I think there needs to be a suite of regular tests that cover many comparisons against control images. Ideally, the dialog itself could have UI tests run usingQTest
, which can modify comboboxes, etc, then a test of the generated image could be compared to control images.To accomplish this, I should probably set up the tricky test suite, then one can continue to add more comparison tests.
@@ -116,6 +176,7 @@ def getSymbolLayer(folder, svg, size): | |||
filepath = os.path.join(base, matching[0]) | |||
break | |||
if filepath is not None: | |||
print('filepath: ' + filepath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Best not to issue print()
commands from a plugin, except for when debugging inside of QGIS, which I'm guessing this is for.
…Added Meteorological symbol sets.
Squashed commits:
[abeb938] Should have all Symbol Sets correctly coded and User experience with the drop downs fully coded and bug free. (fingers crossed.)
[b5b472d] more refactoring
[0a7e0ac] more refactoring
[fde6ed3] more refactoring
[6c8dcf9] more refactoring
[a4ddae0] more refactoring
[8e059cd] more refactoring
[4b27b14] continuing edits to LandUnitSymbolSet
[ec937bc] continuing edits to LandUnitSymbolSet
[11afe22] building out the land unit symbol set.
[7ac134b] building out the land unit symbol set.
[3d788a1] refactoring, creating individual classes for the respective symbol sets. Have updated UI to reflect the workflow of
https://explorer.milsymb.net/#/explore/.
[4a85413] Make SIDC dialog run-able; use old-style ui import for code completion