Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Script order #70

Merged
merged 2 commits into from
Feb 7, 2018
Merged

Script order #70

merged 2 commits into from
Feb 7, 2018

Conversation

mnonnenmacher
Copy link
Contributor

No description provided.

@sschuberth
Copy link
Contributor

Before the Job DSL scripts were processed

Nit: Comma after "Before" in commit message.

* A comparator that orders path names. Files in the same directory are ordered alphabetically, and always come before
* files in subdirectories. Subdirectories are also ordered alphabetically.
*/
class PathComparator implements Comparator<String> {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about this code instead which is a bit shorter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code is not sorting folders alphabetically, you can see in the example output that /games/snake comes after /usr/local, with more files and more folders this would become a total mess.

class PathComparator implements Comparator<String> {
@Override
int compare(String o1, String o2) {
def tokens1 = o1.tokenize('/')
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this use File.separator for portability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had to revert this as it fails on Windows.

def tokens1 = o1.tokenize('/')
def tokens2 = o2.tokenize('/')
for (int i = 0; i < Math.max(tokens1.size(), tokens2.size()); i++) {
def hasNext1 = tokens1.size() > i + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Maybe i + 1 < tokens1.size() is more readable?

int compare(String o1, String o2) {
def tokens1 = o1.tokenize('/')
def tokens2 = o2.tokenize('/')
for (int i = 0; i < Math.max(tokens1.size(), tokens2.size()); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Prefer pre-increment in for loops, ++i.

int compare(String o1, String o2) {
def tokens1 = o1.tokenize(File.separatorChar)
def tokens2 = o2.tokenize(File.separatorChar)
for (int i = 0; i < Math.max(tokens1.size(), tokens2.size()); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Prefer prefix-increment in for-loops, ++i.

def tokens1 = o1.tokenize(File.separatorChar)
def tokens2 = o2.tokenize(File.separatorChar)
for (int i = 0; i < Math.max(tokens1.size(), tokens2.size()); i++) {
def hasNext1 = tokens1.size() > i + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Maybe i + 1 < tokens1.size() reads better?

Before, the Job DSL scripts were processed in whatever order they were
returned from the source set. This made the behaviour unpredictable in
situations were order matters. Therefore define a sorting order for Job DSL
scripts.

Job DSL scripts from the same folder are now executed in alphabetical
order, and before all scripts from subfolders. Subfolders are also
processed in alphabetical order.
@sschuberth sschuberth merged commit 55d7e87 into master Feb 7, 2018
@sschuberth sschuberth deleted the script-order branch February 7, 2018 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants