Code Improvements #118
Code Improvements #118
Conversation
darxriggs
force-pushed
the
improvements
branch
2 times, most recently
from
d56d616 to
bbc3fb1
Compare
|
@batmat is there anything I can help to get this merged? |
|
@darxriggs I just deployed a I.e. like https://github.com/jenkinsci/jenkins/pull/3053/files but with Thanks! |
|
I had to create another pull request (jenkinsci/jenkins/pull/3891) first because of required changes when upgrading jenkins-test-harness (HTMLUnit breaking changes). |
|
@darxriggs sorry, I forgot about the fact you'd be blocked by ongoing fixes for bumping to 2.46 already in progress in jenkinsci/jenkins#3872. I would be inclined to wait for jenkinsci/jenkins#3872 to land so we can test your PR here. Sorry about that |
|
@darxriggs finally, it went much faster than anticipated. Given jenkinsci/jenkins#3872 is now merged, you can move forward by checking #118 (comment) :). thanks! |
|
@batmat the verification with Jenkins core passed. |
| @@ -847,7 +847,7 @@ public UserDetails loadUserByUsername(String username) throws UsernameNotFoundEx | |||
| auths.add(new GrantedAuthorityImpl(g)); | |||
| } | |||
| } | |||
| return new org.acegisecurity.userdetails.User(username,"",true,true,true,true, auths.toArray(new GrantedAuthority[auths.size()])); | |||
| return new org.acegisecurity.userdetails.User(username,"",true,true,true,true, auths.toArray(new GrantedAuthority[0])); | |||
|
|
||
| File dataSetRoot = new File(baseDir,"src/main/preset-data"); | ||
| def dataSetRoot = new File(baseDir, "src/main/preset-data") |
There was a problem hiding this comment.
This definitely looks like a change in the wrong direction to me.
There was a problem hiding this comment.
You refert to def instead of File?
That's reducing type duplication.
Groovy is a dynamic language.
Having types everywhere doesn't provide much more confidence as a lot of things are just checked as runtime unless @CompileStatic is used.
But I can revert this if really desired.
Just let me know.
There was a problem hiding this comment.
I'm not sure on the precise context, but even in Java, this is idiomatic now. For example:
var f = new File(...)versus:
File f = getRoot();Essentially, you should have an obvious type in the line before using def/var.
|
As I answered all review comments I'd like to ask you to have another look and either merge the changes or request specific modifications. |
Mainly removing optional semicolons.
Don't use pre-sized array when calling toArray(). performance-wise pre-sized arrays are not even faster - see https://shipilev.net/blog/2016/arrays-wisdom-ancients/
These types can be inferred by the compiler now.
This reduces boilerplate code.
This reduces boilerplate code.
The Jenkins version, the plugin requires, is already 1.651.2.
darxriggs
force-pushed
the
improvements
branch
from
bbc3fb1 to
90502d9
Compare
|
@darxriggs I just pushed a SNAPSHOT for the current tip Can you please file a similar on the core with it? Once done and green, I'll merge and release here. Thanks a lot for the cleanup! |
|
(sorry if this looks convoluted, we just had a dumpster fire on the previous changes and bump to JTH 2.46, so I'm being extra careful we can then just release a bump the core with latest JTH without outstanding efforts. Thanks for your understanding) |
This is only to verify that the improvements in jenkins-test-harness don't break anything in Jenkins core. See jenkinsci/jenkins-test-harness#118.
These are commits to improve the current code.