-
Notifications
You must be signed in to change notification settings - Fork 147
Action/Generate: Add relocatable option #449
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
base: master
Are you sure you want to change the base?
Conversation
|
Friendly ping |
|
Ping :) Is there anything I can do to move this forward? |
|
@fgaz-scrive could you please rebase to trigger a CI run? |
src/Action/Generate.hs
Outdated
| | [] <- local_ -> do readHaskellOnline timing settings doDownload | ||
| | otherwise -> readHaskellDirs timing settings local_ | ||
| | relocatable, _:_:_ <- local_ -> | ||
| exitFail "Error: --relocatable needs exactly one --local, or the paths will be ambiguous" |
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.
Could we validate this condition earlier? E. g., could our command line interface be --local [PATH] | --relocatable PATH, so that the configuration is valid by construction?
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.
could our command line interface be
--local [PATH] | --relocatable PATH
We'd still have to check that only one of those is supplied, unless cmdargs supports submodes and a breaking change is OK.
I pushed a separate commit with the change so you can compare and possibly revert.
|
@fgaz @fgaz-scrive are you still interested in this? |
|
Yes, I'll follow up next week |
e44d045 to
7de85f0
Compare
This is useful for example if you generate the haddocks and the index in CI and deploy them to another machine at a different path.
7de85f0 to
ecb4b9f
Compare
Bodigrim
left a comment
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! How can I test the new feature?
| | Just _ <- relocatable, _:_ <- local_ -> | ||
| exitFail "Error: --relocatable and --local are mutually exclusive" | ||
| | Just relocatable' <- relocatable -> do | ||
| prefix <- traverse canonicalizePath relocatable |
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.
Could it be
| prefix <- traverse canonicalizePath relocatable | |
| prefix <- canonicalizePath relocatable' |
?
| let url = "file://" ++ ['/' | not $ "/" `isPrefixOf` dir] ++ replace "\\" "/" dir ++ "/" | ||
| let url = case prefixToRemove of | ||
| Just prefix -> makeRelative prefix $ replace "\\" "/" dir ++ "/" | ||
| Nothing -> "file://" ++ ['/' | not $ "/" `isPrefixOf` dir] ++ replace "\\" "/" dir ++ "/" |
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.
Could we please factor out replace "\\" "/" dir ++ "/" so that it's not repeated?
This is useful for example if you generate the haddocks and the index in CI and deploy them to another machine at a different path.
Thanks for the pull request!
By raising this pull request you confirm you are licensing your contribution under all licenses that apply to this project (see LICENSE) and that you have no patents covering your contribution.
If you care, my PR preferences are at https://github.com/ndmitchell/neil#contributions, but they're all guidelines, and I'm not too fussy - you don't have to read them.