Fix incorrect file name regex logic #21
Conversation
- `getDebugFileName` only checks whether '.js' is in the file name. So the 'john.jspiner' case throws an error.
- The file extension must be at the end of the full path.
|
@dbradicich @ataylor284 @thejosh00 |
| private static String getDebugFileName( String file ) { | ||
| String result = file.replace( ".js", "-debug.js"); | ||
| return result; | ||
| int extensionIndex = file.lastIndexOf( ".js" ); |
There was a problem hiding this comment.
file.replaceFirst(".js$", "-debug.js"); might be a better way to express this.
There was a problem hiding this comment.
Much better! 👍
Applied in 9eb5223
|
|
||
| public class CodeFileGeneratorTest { | ||
|
|
||
| private String getDebugFileName(String file) { |
There was a problem hiding this comment.
I'd just make getDebugFileName package protected (no modifier) instead of using reflection to invoke for testing.
There was a problem hiding this comment.
I was confused because I didn't know the test rules of the project, but it's good that I can use protected function. 👍
Applied in 68d3969
There was a problem hiding this comment.
@JSpiner Thanks for pushing up a UT as well :) we are going to be doing a new release to use in nxrm 3.37 soon and would surely like to include this for you after the comments get resolved
directjngine/src/test/java/com/softwarementors/extjs/djn/jscodegen/CodeFileGeneratorTest.java
Show resolved
Hide resolved
- Remove reflection test. - Thanks to @ataylor284
- And reformat code. - Thanks to @dbradicich
|
Hi @JSpiner - looks like the code is ready for us to merge. Before we can do that, we need to you sign our Contributor License Agreement, found here: https://sonatypecla.herokuapp.com/sign-cla Once you're signature is in we can merge and include in the next release - thank you! |
|
Please attach a screenshot of the completed form to a comment on this thread. |
There was a problem hiding this comment.
Looks good to me, I almost forgot the CLA needs to signed prior to accepting the PR, but codewise looks good to me :)
Please see this link https://sonatypecla.herokuapp.com/sign-cla for the CLA, just comment back here when done and we should be able to get merged for next release (earliest release would be 3.37 of nxrm fyi)
edit: nvmd, i see mr blair beat me to it :)
|
I signed with this github account. ([email protected]) |
|
Is there anything more should I do? |
|
@ataylor284 @dbradicich |
|
Merged, but it won't get included in Nexus Repository until the next release. I'm not sure when that will be but we missed getting this into 3.37 unfortunately. I believe you could also build this locally and replace the jar in your nexus instance while you wait for a new release. |
When I run nexus with
nexus run, I got the following error.My user name is
smith.jspinerand file path is/Users/smith.jspiner/nexus-......But strangely, nexus was trying with
/Users/smith-debug.jspiner/nexus-..... 😭When I checked the source code, there was an error in changing the file name for debug.
I modified the regex logic and added the test code.
Please review it. 🙏