-
Notifications
You must be signed in to change notification settings - Fork 507
Support scala-steward.conf
(makes sense in .github
and .config
folder)
#3115
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
Support scala-steward.conf
(makes sense in .github
and .config
folder)
#3115
Conversation
scala-steward.conf
(makes sense in .github and .config folder)scala-steward.conf
(makes sense in .github
and .config
folder)
c09fce1
to
ebd63e0
Compare
val configFileCandidates: F[List[File]] = (repoConfigFileSearchPath | ||
.map(_ :+ repoConfigBasename) ++ | ||
repoConfigFileSearchPath | ||
.map(_ :+ repoConfigBasename.substring(1))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix is here: The .substring(1)
removes the leading dot from the filename.
As you can see I just append the possible non-hidden config file locations to the end of the list, which later will be searched for the first existing config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to change the repoConfigBasename to be without the .
instead, and then add the dot here? substring use here adds a bit of a mental overhead.
PS: not associated with the project, but got a notification because of my PR and had to look at the code! Feel free to ignore :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repoConfigBasename
variable is publicly accessible therefore I don't want to change it's value for backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line 46 of this file it maybe good to use the actual filename (activeConfigFile
) instead of repoConfigBasename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mzuehlke Done and pushed, thanks!
assert(fileAlg.isRegularFile(dotGithubConfigFile).unsafeRunSync()) | ||
|
||
assert(config.maybeRepoConfig.isDefined) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests I added are basically just duplicates of the preceding test(s), just with the dot removed from the filename.
@fthomas @alejandrohdezma @mzuehlke if one of you can review, I think the PR is very straight forward, tests are all green, would love to see it merged, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comment for a better exception message.
val configFileCandidates: F[List[File]] = (repoConfigFileSearchPath | ||
.map(_ :+ repoConfigBasename) ++ | ||
repoConfigFileSearchPath | ||
.map(_ :+ repoConfigBasename.substring(1))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line 46 of this file it maybe good to use the actual filename (activeConfigFile
) instead of repoConfigBasename
ebd63e0
to
bff84e2
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #3115 +/- ##
=======================================
Coverage 91.03% 91.03%
=======================================
Files 162 162
Lines 3404 3404
Branches 311 311
=======================================
Hits 3099 3099
Misses 305 305
☔ View full report in Codecov by Sentry. |
Without the leading dot (not hidden),
Now that the config can be located inside the already hidden
.github
and.config
folder (#3033) there is not need for the config file to be hidden as well inside that folders.To stay backward compatible a hidden config
.scala-steward.conf
will always be preferred, only when such a hidden file can't be found anywhere, only then we try to read the "unhidden"scala-steward.conf
in the same search order (root,.github
,.config
folders)See #3033 (comment)