-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix: Correction of the siteRoot path #17297
Conversation
- Add a getOrElse to not only take a value that does not match the default.
8dcf2e3
to
7a35540
Compare
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.
Thanks for the pr @Dedelweiss! Is there any existing tests for passing in the siteRoot
? If so it might be a good idea to add one passing in the default here just to show that this working as expected. Could you check this and get back?
Hi, in fact according to my research, I did not find any tests corresponding to the modifications of my RP. I also don't know how to test this. Knowing that I should not modify the Scaladoc.scala. If you have any ideas I am interested, thank you very much. |
@Florian3k maybe you have some ideas here about an easy way to test this? |
@ckipp01 Unfortunately, I don't have any. |
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.
I'm honestly ok with just merging this. It makes sense and does seem to fix the issue. Creating an entire test setup to try and test this seems a bit unrealistic. Ideally there would already be a framework for this, but since there's not I don't really expect you to do it. This is a LGTM from me, but I'd also like @Florian3k to verify.
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.
I think Option(siteRoot.withDefault(siteRoot.default))
is equivalent to Option(siteRoot.get)
, but thats minor issue.
Otherwise LGTM
- For the first issue, when a user set ./docs to the default value of ScalaSettings, the code only took a value that was not the default `siteRoot.nonDefault`. To fix this, I put a getOrElse to try and get the default value. - For the second problem, I noticed when I cloned the code that the user was calling layouts that did not exist. Be careful with this as it can indeed produce an error. Solving this problem was enough to remove the non-existent layout calls. Fixes: #15306 [Cherry-picked 4e4552e]
Backports #17297 to the LTS branch. PR submitted by the release tooling.
For the first issue, when a user set ./docs to the default value of ScalaSettings, the code only took a value that was not the default
siteRoot.nonDefault
. To fix this, I put a getOrElse to try and get the default value.For the second problem, I noticed when I cloned the code that the user was calling layouts that did not exist. Be careful with this as it can indeed produce an error. Solving this problem was enough to remove the non-existent layout calls.
Fixes: #15306