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

Codacy recommended cleanup. #524

Merged
merged 1 commit into from
Mar 17, 2015
Merged

Codacy recommended cleanup. #524

merged 1 commit into from
Mar 17, 2015

Conversation

myyk
Copy link
Contributor

@myyk myyk commented Mar 17, 2015

This should not be a functional change. It is just some of recommended clean up from https://www.codacy.com/public/nepomukseiler/sbt-native-packager/dashboard

I was just snooping and figured, might as well fix a couple.

@muuki88
Copy link
Contributor

muuki88 commented Mar 17, 2015

Nice. Thanks :)

muuki88 added a commit that referenced this pull request Mar 17, 2015
Codacy recommended cleanup.
@muuki88 muuki88 merged commit 0d72913 into sbt:master Mar 17, 2015
@muuki88
Copy link
Contributor

muuki88 commented Mar 17, 2015

There is a change in logic, which broke the tests. However I'm not sure why the windows tests only run on my PRS.

Can you provide a second pull request?

@muuki88
Copy link
Contributor

muuki88 commented Mar 17, 2015

The error

if (f == null) Seq() else Seq(f) ++ allParentDirs(f.getParentFile)
// with braces
if (f == null) { Seq() } else { Seq(f) ++ allParentDirs(f.getParentFile) }

// so this would be
Option(f).map(f => f :+ allParentDirs(f.getParentFile) ) getOrElse Seq()

@myyk
Copy link
Contributor Author

myyk commented Mar 17, 2015

Oh, sorry about that. Yeah, that's clearer with braces. What do you think about this? It preserves the ordering as well.

Option(f).toSeq.flatMap(f => f +: allParentDirs(f.getParentFile))

@myyk
Copy link
Contributor Author

myyk commented Mar 17, 2015

I wasn't sure how to test my own change. I think I now have appveyor setup correctly so that I can test the fix myself before making the pull request this time.

@myyk myyk mentioned this pull request Mar 17, 2015
@muuki88
Copy link
Contributor

muuki88 commented Mar 18, 2015

Oh xD This is a misconfiguration on our side. Have to check when I'm back in Germany why appveyor is no running on pull request that don't come from this repo. However thanks for testing this 👍

LGTM

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