[OMSC-1266] Ninja Printer version check #8
Conversation
…on with each request. Modified the extension to react on the version
|
@peteraba Please do the first review. |
| @@ -4,14 +4,16 @@ var NinjaPrinter = { | |||
| events: { | |||
| beforePrint: "ninjaprinter.beforePrint", | |||
| afterPrint: "ninjaprinter.afterPrint", | |||
| result: "ninjaprinter.result" | |||
| result: "ninjaprinter.result", | |||
| versionWarning: "ninjaprinter.versionWarning" | |||
There was a problem hiding this comment.
minor Maybe we should add trailing comma. Chrome should be fine with that.
| }, | ||
|
|
||
| // Attached cutom events listeners map | ||
| listeners: { | ||
| "ninjaprinter.result": [], | ||
| "ninjaprinter.afterPrint": [], | ||
| "ninjaprinter.beforePrint": [] | ||
| "ninjaprinter.beforePrint": [], | ||
| "ninjaprinter.versionWarning": [] |
There was a problem hiding this comment.
minor Maybe we should add trailing comma. Chrome should be fine with that.
There was a problem hiding this comment.
isn't that invalid in JS?
| @@ -1,3 +1,69 @@ | |||
| class SemVer { | |||
| constructor(version) { | |||
| let semVerRegex = /(\d)+\.(\d)+\.(\d)+\-?\d*\-?g?([0-9a-f]*)/; | |||
There was a problem hiding this comment.
Please add some comments for this.
|
|
||
| let result = version.match(semVerRegex); | ||
|
|
||
| if (result === null) |
There was a problem hiding this comment.
minor opening braces on same line as the if keyword please.
| this.commit = parseInt(result[4]); | ||
| } | ||
|
|
||
| get Major() { |
There was a problem hiding this comment.
where's this style from? doesn't look like the rest of our codebase.
|
|
||
| if (difference !== 0) { | ||
| let message = `The NinjaPrinter extension and host versions do not match.\n | ||
| The extension version is ${difference < 0 ? 'lower' : 'higher'} than the host.`; |
There was a problem hiding this comment.
please increase the indentation here.
| <exec executable="git" outputproperty="build.version" failonerror="true"> | ||
| <arg value="describe"/> | ||
| </exec> | ||
| <replaceregexp file="../java/../chrome-extension/browser/manifest.json" |
There was a problem hiding this comment.
the following 2 tags are impossible to unravel, so please add some comment(s) describing them.
| @@ -121,7 +125,8 @@ public void start() { | |||
| */ | |||
| protected MessageInterface getSuccessMessage(JsonPrintMessageInterface printMessage) | |||
| { | |||
| String response = "{\"success\":true,\"message\":\"success\", \"requestId\":\"" + printMessage.getRequestId() + "\"}"; | |||
| String response = "{\"success\":true,\"message\":\"success\", \"requestId\":\"" + printMessage.getRequestId() + | |||
There was a problem hiding this comment.
note this is (still) a bit ugly, but I can live with that.
There was a problem hiding this comment.
@peteraba Can we make it a follow up ticket to change this to use a JSON serialisation library?
| try { | ||
| String jarLocation = NinjaPrinter.class.getProtectionDomain().getCodeSource().getLocation().toURI().getPath(); | ||
|
|
||
| Manifest m = new JarFile(jarLocation).getManifest(); |
There was a problem hiding this comment.
it's gonna be interesting to write unit test(s) for. 😉
There was a problem hiding this comment.
I know 😞
@peteraba Can we add a follow up ticket to add unit testing and to refactor the code?
|
@wwgerner please do the last review. |
| }, | ||
|
|
||
| // Attached cutom events listeners map | ||
| listeners: { | ||
| "ninjaprinter.result": [], | ||
| "ninjaprinter.afterPrint": [], | ||
| "ninjaprinter.beforePrint": [] | ||
| "ninjaprinter.beforePrint": [], | ||
| "ninjaprinter.versionWarning": [] |
There was a problem hiding this comment.
isn't that invalid in JS?
| @@ -1,3 +1,69 @@ | |||
| class SemVer { | |||
| @@ -1,7 +1,9 @@ | |||
| { | |||
| "name": "Ninja Printer", | |||
| "version": "1.0.5", | |||
| "version": "1.0.3", | |||
There was a problem hiding this comment.
So our newer version with the version handling has a lower version? :)
There was a problem hiding this comment.
Haha, I checked in the version populated by the build script on my machine. It gets overwritten based on which tag you're working in.
|
@CanLikeTo Please check the comments, nice development! |
|
LGTM |
|
@CanLikeTo If you want to add followups, feel free to add them :) |
No description provided.