Iatr/stop

[it-hms]

Closes #70, Updates dependencies, and corrects/updates tasks.json

[alexjhawk]

Wouldn't this be more appropriately located in the cleanup() method?

[it-hms]

This is what I thought. I wanted to interrupt and join the event handler thread. However, the ETK does not provide this ( or I could not find it).
This is sort of a hack solution. But I don't like the call in cleanup. If anyone ever added a function after cleanup it would never run.

[alexjhawk]

I would rather see this added to the cleanUp() method called right above since things like this are what it was intended for. (Of course, the System.exit call would probably be at the end)

We don't need to worry about whether someone adds a function after cleanup. The calls to shutDown() and cleanUp() are intended to be the last things called in the application, and nobody should add anything after them.

[it-hms]

But its not obvious, or nowhere is it stated that nothing should be called after cleanUp. For example, the AbstractConnectorMain has a postCleanUp method that is called after.

[alexjhawk]

It could be clarified better that nothing is intended to go after cleanup. The intention of shutDown() and cleanUp(), though, was to isolate any of the shutdown and cleanup logic to a location outside of the main method (to avoid a long, cluttered main method).

Regarding AbstractConnectorMain, the same reasoning still stands, although the abstract connector framework was developed more recently, and the postCleanUp() method was an addition to help facilitate a cleaner connector/device restart procedure.

Ultimately, when time permitted, my goal was to migrate this Cumulocity connector to use the abstract connector framework. However, although this connector is similar to the abstract connector framework, there are minor differences and ambiguities, and the framework should be considered the 'better one' structure-wise.

[it-hms]

@alexjhawk ok. I will add to cleanUp and add comments

[alexjhawk]

These changes do not align with the current starter project version: https://github.com/hms-networks/sc-java-maven-starter-project/blob/main/.vscode/tasks.json.

Generally, a starter project-based file change like this should be made in the starter project first and then brought downstream.

See previous review discussion about this topic: #55 (comment) and #55 (comment)

[alexjhawk]

I am following up on this because there has previously been back-and-forth regarding this:

• The change was introduced ~10 months ago in the starter project via commit from @TomKimsey.
  • hms-networks/sc-java-maven-starter-project@6cee88d
• The change ensures compatibility with Powershell.

As a potential compromise solution, does it make sense to duplicate the tasks and create Powershell and Bash variants? This would allow use regardless of whether the developer uses Powershell or Bash. Of course, the change(s) should still be introduced in the starter project, but this compromise solution would hopefully resolve the incompatibilities between individual developer setups.

[it-hms]

will remove

[it-hms]

I dropped these changes

[alexjhawk]

Ok. Although I have not historically used VS Code for Ewon Java development, I do think this needs to be addressed soon because it has come up several times. At this point, though, it's still a starter project-level concern, so it doesn't need to be added or taken care of now/here.

[it-hms]

The current tasks are just setup for Powershell as the default shell. I use git bash as default and most of the commands are not compatible. Strangely if you set windows cmd as the default shell, the tick also cause problems.
There is really no simple way to accommodate the two different default shells.

[TomKimsey]

Yeah, i struggled to find a good solution for this problem other than just adding another file and documenting that you would need to do some file renaming if you wanted to use a bash or cmd compatible file. I went with PowerShell as it would be the default for VS Code users.

[it-hms]

@TomKimsey ,
Good idea on the file renaming.
I propose we change this file name to tasks_powershell.json, create a tasks_bash.json. Add .vscode/tasks.json to .gitignore and remove from tracking.
I will add issue to extlib.

[alexjhawk]

I don't personally use VS Code development for Java, but I would think requiring developers to manually rename files should be a last resort.

It's also not a clean procedure, because there is a sizable chance that the developer would open the project in VS Code and have to do the renaming there, which might require a restart of VS Code to pick up.

Why not just add different entries in a single .vscode/tasks.json like below? It would make the development environment much more user-friendly during setup but achieve the same functionality.

{
    "version": "2.0.0",
    "tasks": [
        {
            "label": "INSTALL (Upload to Device) - Powershell",
            "type": "shell",
            "command": "mvn install -f pom.xml `-Dmaven.javadoc.skip=true",
            "group": "build"
        },
        {
            "label": "INSTALL (Upload to Device) - Bash",
            "type": "shell",
            "command": "mvn install -f pom.xml -Dmaven.javadoc.skip=true",
            "group": "build"
        },
        {
            "label": "PACKAGE (Create JAR)",
            "type": "shell",
            "command": "mvn package -f pom.xml",
            "group": "build"
        },
        {
            "label": "DEPLOY (Run on Device, No Debug) - Powershell",
            "type": "shell",
            "command": "mvn deploy -f pom.xml -P noDebug `-Dmaven.javadoc.skip=true",
            "group": "build"
        },
        {
            "label": "DEPLOY (Run on Device, No Debug) - Bash",
            "type": "shell",
            "command": "mvn deploy -f pom.xml -P noDebug -Dmaven.javadoc.skip=true",
            "group": "build"
        },
        ...
    ]
}

[it-hms]

This is clutter. Renaming a file is not a problem. Also every developer uses tasks differently.
I am going to add a format task and a clean task that only removes target/classes.
There is no reload required