Refactor tty #1546 #1581
Refactor tty #1546 #1581
Conversation
move tests to TtyInterface from Startup
single point of truth for "is GUI?" refactor Startup, TtyInterface, GuiInterface Startup mostly only parses arguments TtyInterface handles all CLI operations GuiInterace starts up GUI some tests for Startup
added some tests
introduce Task enum
template vars
regroup arg handling
dtowell
requested review from
BFH-ktt1,
maehne,
MarcinOrlowski and
zdimension
as code owners
There was a problem hiding this comment.
Thanks @dtowell for all your work and @MarcinOrlowski for your review! It goes for me into the right direction.
@dtowell: Do you consider this PR still a draft or being in a shape for integration into develop?
| } | ||
| logger.error(S.get("invalidLocaleError")); | ||
| logger.error(S.get("invalidLocaleOptionsHeader")); | ||
| for (final var o : opts) { |
There was a problem hiding this comment.
Personally, I don't have problems with single letter names for local variables - especially if the context in which they are used/valid is small. I agree that argument names, member variables, constants, and other identifiers, which are visible in larger scopes should have longer meaningful names. E.g., I see on object S used all over the place in this file and others of this PR, which is IMHO not good practice.
|
@dtowell Please click "Resolve conversation" buttons for each change request you consider handled (in any way). It's appreciated if you add a comment (even like "Done") for changes you made as requested. If you disagree (and change is not made, please comment with your reasoning so we can proceed). You may keep non changed requests opened so it will be easier for a reviewer to jump in and either close by himself (if you made a point) or keep the discussion going. Thanks. |
|
@MarcinOrlowski ok, thanks for explaining |
|
@MarcinOrlowski: What is open from your side before this PR can be merged? |
|
@dtowell: There are some conflicts in the localisation strings, which need to be resolved. Could you please have a look? Easiest should be to either rebase this PR on |
This refactors startup and verifies command-line parameter combinations to prevent unexpected results when options are allowed but later ignored. It also moves all non-GUI operations into TtyInterface.
There are now exactly 3 modes of startup:
Possible backward compatibility breaking changes:
Also many tests were added. Addition tests are sorely needed but are very hard to write since they require mocking constructors. Much of the code needs to be refactored to use dependency injection to allow more unit testing.
Some option inconsistencies remain. For example. sometimes options include the .circ file as a required parameter and sometime it is required but not an option parameter. Issues and other PRs will be created to discuss individual items.