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

[WIP] Porting OctoTools to FAKE 5 #2048

Merged
merged 8 commits into from Aug 18, 2018

Conversation

@queil
Copy link
Contributor

commented Aug 3, 2018

Description

An initial PR just to get some feed back.

TODO

  • New (API-)documentation for new features exist (Note: API-docs are enough, additional docs are in help/markdown)
  • [] unit or integration test exists (or short reasoning why it doesn't make sense)
  • (if new module) the module has been linked from the "Modules" menu, edit help/templates/template.cshtml, linking to the API-reference is fine.
  • (if new module) the module is in the correct namespace
  • (if new module) the module is added to Fake.sln (dotnet sln Fake.sln add src/app/Fake.*/Fake.*.fsproj)
  • Fake 5 API guideline is honored
@queil queil referenced this pull request Aug 3, 2018
@matthid
Copy link
Collaborator

left a comment

Looks quite good already

| Push of PushOptions

/// Complete Octo.exe CLI params
[<CLIMutable>]

This comment has been minimized.

Copy link
@matthid

matthid Aug 4, 2018

Collaborator

Generally we remove CLIMutable is that required here?

This comment has been minimized.

Copy link
@queil

queil Aug 4, 2018

Author Contributor

I do not think so. Will double-check and remove if not.



/// Default server options.
let serverOptions = { Server = ""; ApiKey = ""; }

This comment has been minimized.

Copy link
@matthid

matthid Aug 4, 2018

Collaborator

typically we set options via callbacks, therefore we usually mark defaults "private" or "internal" as they are not really required by the user

let serverOptions = { Server = ""; ApiKey = ""; }

/// Default options for 'CreateRelease'
let releaseOptions = {

This comment has been minimized.

Copy link
@matthid

matthid Aug 4, 2018

Collaborator

Same here (and others)

WorkingDirectory = "" }

/// [omit]
let optionalStringParam p o =

This comment has been minimized.

Copy link
@matthid

matthid Aug 4, 2018

Collaborator

Such helpers should all be private/internal if not used by the consumer

/// ## Parameters
///
/// - `setParams` - Function used to overwrite the OctoTools default parameters.
let Octo setParams =

This comment has been minimized.

Copy link
@matthid

matthid Aug 4, 2018

Collaborator

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.

This comment has been minimized.

Copy link
@queil

queil Aug 4, 2018

Author Contributor

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 ...}

This comment has been minimized.

Copy link
@matthid

matthid Aug 4, 2018

Collaborator

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.

This comment has been minimized.

Copy link
@queil

queil Aug 4, 2018

Author Contributor

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.

@queil

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2018

Thanks for the feed back. I will fix the issues soon.

queil added 2 commits Aug 4, 2018
Making public API consistent with FAKE 5 guidelines.
Making Channel available in deployRelease and deleteReleases.
@matthid

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2018

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.

@queil

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2018

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.

@queil

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2018

@matthid I am happy with the code now.

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2018

@queil Yes looks good, just the couple todos left from the PR todo list (some organizational and documentation stuff).

@queil

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2018

Cool, I'll go through that tomorrow. Thanks.

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Aug 11, 2018

ping :)

queil added 3 commits Aug 11, 2018
@queil

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2018

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?

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Aug 18, 2018

I looked through it twice and I don't think there is anything to add here, good job and thanks a lot!

@matthid matthid merged commit b77828e into fsharp:release/next Aug 18, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.