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

Refactoring and testing complete. (+13 squashed commits) #15

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Refactoring and testing complete. (+13 squashed commits) #15

wants to merge 3 commits into from

Conversation

vickdw
Copy link

@vickdw vickdw commented Mar 25, 2020

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

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
@vickdw vickdw requested a review from dakcarto March 25, 2020 18:32
Copy link
Contributor

@dakcarto dakcarto left a 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, maybe SymbolSet, 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 to 11 Military for code 10010535221100000000):
    mil-std-2525_symbol-not-same

  • Then, after setting Entity to 11 Military, and then trying to set it back to 00 Not Applicable, this error is thrown:
    mil-std-2525_str-key-err

  • 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 using QTest, 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)
Copy link
Contributor

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.

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.

2 participants