Skip to content

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

Open
vickdw wants to merge 3 commits into
planetfederal:masterfrom
vickdw:GEOIN-122
Open

Refactoring and testing complete. (+13 squashed commits)#15
vickdw wants to merge 3 commits into
planetfederal:masterfrom
vickdw:GEOIN-122

Conversation

@vickdw

@vickdw vickdw commented Mar 25, 2020

Copy link
Copy Markdown

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

@dakcarto dakcarto left a comment

Copy link
Copy Markdown
Contributor

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.

filepath = os.path.join(base, matching[0])
break
if filepath is not None:
print('filepath: ' + filepath)

Copy link
Copy Markdown
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