[WIP] Porting OctoTools to FAKE 5 #2048
Conversation
src/app/Fake.Tools.Octo/Octo.fs
Outdated
| | Push of PushOptions | ||
|
|
||
| /// Complete Octo.exe CLI params | ||
| [<CLIMutable>] |
There was a problem hiding this comment.
Generally we remove CLIMutable is that required here?
There was a problem hiding this comment.
I do not think so. Will double-check and remove if not.
src/app/Fake.Tools.Octo/Octo.fs
Outdated
|
|
||
|
|
||
| /// Default server options. | ||
| let serverOptions = { Server = ""; ApiKey = ""; } |
There was a problem hiding this comment.
typically we set options via callbacks, therefore we usually mark defaults "private" or "internal" as they are not really required by the user
src/app/Fake.Tools.Octo/Octo.fs
Outdated
| let serverOptions = { Server = ""; ApiKey = ""; } | ||
|
|
||
| /// Default options for 'CreateRelease' | ||
| let releaseOptions = { |
src/app/Fake.Tools.Octo/Octo.fs
Outdated
| WorkingDirectory = "" } | ||
|
|
||
| /// [omit] | ||
| let optionalStringParam p o = |
There was a problem hiding this comment.
Such helpers should all be private/internal if not used by the consumer
src/app/Fake.Tools.Octo/Octo.fs
Outdated
| /// ## Parameters | ||
| /// | ||
| /// - `setParams` - Function used to overwrite the OctoTools default parameters. | ||
| let Octo setParams = |
There was a problem hiding this comment.
We would choose a different name as currently the usage is Octo.Octo in fake 5 we opt for Octo.run or Octo.exec or whatever the correct verb is.
There was a problem hiding this comment.
The full list of Octo.exe verbs is quite long. Shouldn't I create many verbs instead of just run:
let deployRelease setParams = ...
let createRelease setParams = ...Called like:
Octo.deployRelease (fun ps -> {ps with ...}
Octo.createRelease (fun ps -> {ps with ...}
There was a problem hiding this comment.
That depends on the representation of the setParams type. Both styles are valid according to the fake 5 guideline. I guess if the verbs are quite different then different functions with different parameter records is easier for users. If parameters are very equal across the different verbs than sharing (or only having a single function) makes more sense.
I'll leave that up to you as I'm not familiar with octo.exe.
There was a problem hiding this comment.
Thanks, there is only a handful of common options. Also the verbs do actually do quite different things. So I will go for separate verbs then.
|
Thanks for the feed back. I will fix the issues soon. |
Making Channel available in deployRelease and deleteReleases.
|
Also note: There is no need to implement all verbs right now. It's completely acceptable to only implement the subset you need "right now" or we supported previously? If somebody needs an additional verb we can add them on-demand or he can send its own PR. |
|
Yes, I agree. I only added whatever was implemented there already. The only new feature is adding Channel parameter to deployRelease and deleteReleases which I need "right now" indeed. |
|
@matthid I am happy with the code now. |
|
@queil Yes looks good, just the couple todos left from the PR todo list (some organizational and documentation stuff). |
|
Cool, I'll go through that tomorrow. Thanks. |
|
ping :) |
|
I've just committed a couple of changes regarding the docs. The only missing point is unit/integration tests. I do not think it make much sense in this kind of tool. The only tests I can think of would be to verify whether for a given set of parameters a correct command string is generated. What do you think? |
|
I looked through it twice and I don't think there is anything to add here, good job and thanks a lot! |
Description
An initial PR just to get some feed back.
TODO
help/markdown)help/templates/template.cshtml, linking to the API-reference is fine.dotnet sln Fake.sln add src/app/Fake.*/Fake.*.fsproj)