Skip to content
This repository has been archived by the owner on Mar 27, 2024. It is now read-only.

Changed CommandConfiguration and Command Design #407

Merged
merged 11 commits into from Sep 4, 2019

Conversation

osbornjd
Copy link

Summary

This PR introduces a change in the design of CommandConfiguration and Command. Much of the functionality of Command was moved into CommandConfiguration. This cleans up Command significantly and also is better design for putting all of the work prior to running the command into CommandConfiguration.

This PR supports issues 405 and 406

Joe Osborn and others added 6 commits August 29, 2019 13:54
...could build correctly without throwing "errors" that were the result
of yet unimplemented tests. Also updated pom file so that maven build
functions in command line.

Signed-off-by: Joe Osborn <osbornjd@ornl.gov>
Signed-off-by: Jay Jay Billings <billingsjj@ornl.gov>
Finished some work started yesterday on filling out some of the remote
command member variables/methods

Signed-off-by: Joe Osborn <osbornjd@ornl.gov>
Reconfigured CommandConfiguration to remove the dictionary and be a
POJO. This required adjusting much of the Command design. Also added a
ConnectionConfiguration to the constructor since a command should have a
connection associated with it by default (whether that be local/remote).

Signed-off-by: Joe Osborn <osbornjd@ornl.gov>
This commit overhauls the command design structure. Much of the
functionality was put into CommandConfiguration, and some other things
were reorganized to clean up Command. Additionally, the dictionary was
removed in CommandConfiguration and now strings are passed to configure
the command.

Signed-off-by: Joe Osborn <osbornjd@ornl.gov>
Signed-off-by: Joe Osborn <osbornjd@ornl.gov>
@osbornjd
Copy link
Author

osbornjd commented Aug 30, 2019

I had to rebase again because there was somehow a merge commit that still didn't have the sign off signature (even though I did the merge in eclipse). I'll have to figure out how to be able to do merges in eclipse and force it to add the sign off signature.

It looks like all my commits stayed as is, and git just replaced the merge commit with @jayjaybillings FileHandler commit (which is what the merge commit was dealing with anyway). Note that I haven't worked on FileHandler today, so there are no changes since @jayjaybillings commit yesterday.

Copy link
Member

@jayjaybillings jayjaybillings left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. All of the CommandConfiguration member variables that you are calling directly in Command, LocalCommand, and CommandFactory should be handled with getters and setters instead of directly accessing them. I think it makes sense for them to be protected, but we generally don't take that as license to access them directly from within the same package.
    The reason for this is that we need to encapsulate the logic of how we manage them. This would, for example, let us do things like strip bad characters or otherwise modify configuration properties without exposing them to the user, (c.f. - getExecutableName()). Likewise, it will also let us do things like store all the properties in a map instead of as member variables if we so choose.
  2. We need to get rid of the nasty constructors for CommandConfiguration. Constructors with that many arguments in lieu of explicit setters are bad practice. That is: dump everything except the nullary constructor and use setters instead.
  3. setConfiguration should probably go on Command instead of CommandConfiguration. That code just initialized things needed for Command. This means createOutputFile should go back to Command too, but that's OK since we can manipulate the file handles on CommandConfiguration by reference.
  4. You need to implement CommandConfigurationTest. Remember in TDD we do this first.
  5. You can delete CommandStatusTest. We don't need it for a simple enumeration, but...
  6. We need documentation on the enumerated values in CommandStatusTest.
  7. You need to put all of the '''fail("Not yet implemented");''' statements back into the code. If you want Maven to build without encountering these errors, then you need to tell Maven to do so by passing the '''-DskipTests''' argument. It may be easiest to identify all of these tests and revert them back to the previous state with Git instead of doing it manually (although it isn't a lot).

One good change that I think you did is put the stdout and stderr handles on CommandConfiguration. I didn't initially like this, but it is actually quite good and it solves and old problem that I'll describe another time. Good job there!

Joe Osborn added 4 commits September 3, 2019 08:34
Signed-off-by: Joe Osborn <osbornjd@ornl.gov>
Signed-off-by: Joe Osborn <osbornjd@ornl.gov>
Signed-off-by: Joe Osborn <osbornjd@ornl.gov>
Signed-off-by: Joe Osborn <osbornjd@ornl.gov>
@osbornjd
Copy link
Author

osbornjd commented Sep 3, 2019

The previous 4 commits should address the concerns/suggestions brought up by @jayjaybillings above from 3 days ago. The requested changes have been implemented in these previous 4 commits.

Copy link
Member

@jayjaybillings jayjaybillings left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to document all the accessors in CommandConfiguration.

Signed-off-by: Joe Osborn <osbornjd@ornl.gov>
@osbornjd
Copy link
Author

osbornjd commented Sep 3, 2019

The previous commit adds some comments to each of the get/set pairs in CommandConfiguration

Copy link
Member

@jayjaybillings jayjaybillings left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved for now. I'm not entirely sure that I like the way you added comments to the accessors because it is against the style of the project. For other projects, I have started thinking about doing that lately, but for this project it is against the style guidelines. Not a blocker though.

@jayjaybillings
Copy link
Member

Thanks for all the changes.

All checks pass and @osbornjd is covered by the UT-Battelle MLA. Accepted. Thanks!

@jayjaybillings jayjaybillings merged commit 31f798d into eclipse:jay/MarkIII Sep 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants