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

Refactor GitHub API #1726

Merged
merged 4 commits into from
Oct 28, 2017
Merged

Refactor GitHub API #1726

merged 4 commits into from
Oct 28, 2017

Conversation

csmager
Copy link
Contributor

@csmager csmager commented Oct 26, 2017

The primary change here is to allow the user to specify the release property values explicitly when creating a GitHub release. The previous createDraft / createRelease API enforced that the release name matched the tag name and that the body was set to a newline delimited set of release notes.

CreateRelease uses a similar pattern to a lot of Fake modules in that you can override the default params where required.

CreateDraftWithNotes DraftNewRelease keeps the previous opinionated behaviour of createDraft for users who are happy with that.

As this is a breaking change and Fake 5 is as-yet unreleased, I also pascal cased the functions to be more consistent with the rest of the Fake modules. Happy to revert this if the prevailing opinion is to the contrary.

Closes #1723

- Changed CreateRelease to allow the user to explicitly set each of the
  release parameters on creation.
- Add CreateDraftWithNotes with same signature as old createDraft func
- Add / fix comments, ensure param names are consistent
- Change functions to Pascal Case
@csmager
Copy link
Contributor Author

csmager commented Oct 26, 2017

Will need some advice on how to fix the build given this is a breaking change to the API. If I change build.fsx to use the new API it won't run at all & if I don't change it then it will fail to bootstrap itself.

@matthid
Copy link
Member

matthid commented Oct 26, 2017

When bootstrapping you can assume that BOOTSTRAP is defined, therefore #if BOOSTRAP should work.

@csmager
Copy link
Contributor Author

csmager commented Oct 26, 2017

OK - and then we remove the conditionals once merged? Sounds good. Sorry, a bit new to this!

@matthid
Copy link
Member

matthid commented Oct 26, 2017

No problem, actually this is quite new as I was running into the same problem several times already ;)

/// - `tagName` - the name of the tag to use for this release
/// - `setParams` - function used to override the default release parameters
/// - `client` - GitHub API v3 client
let CreateRelease owner repoName tagName setParams (client : Async<GitHubClient>) =
Copy link
Member

@matthid matthid Oct 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question is if we should add owner repoName and tagName to the record as well. It "might" help to provide same script internal defaults if you create releases to multiple repos with the same owner.

I don't have a strong opinion on that, just something that crosses my mind. Question is if we can provide reasonable defaults for any of those parameter (but we already have APIs which have mandatory arguments in the record without default)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had considered, but thought as we had no defaults & they were required that it was better to explicitly call out they were required. I suppose it's nice that you could do CreateRelease "owner" "repo" "v1.0" id ... and it would just work.

@csmager
Copy link
Contributor Author

csmager commented Oct 26, 2017

Struggling a bit with this bootstrapping. I've got the #if BOOTSTRAP condition, and the new code is ignored on first run as expected. I can see the call in the log to run the script again:

build/FAKE.exe testbuild.fsx PrintColors --fsiargs "--define:BOOTSTRAP" -pd

But it's failing with messages that imply it doesn't have BOOTSTRAP defined at all.

testbuild.fsx(930,16): error FS0039: The value, constructor, namespace or type 'createClientWithToken' is not defined. Maybe you want one of the following:
    CreateClientWithToken
    CreateClient
    CreateGHEClientWithToken
    CreateGHEClient
    CreateDraftWithNotes

I've pushed the change anyway.

@csmager
Copy link
Contributor Author

csmager commented Oct 26, 2017

OK, I think I've found it. -pd isn't an option in Cli.fs for netcore. It seems -pd was 'print details' in the old cli. If I change to -p, it works. If I remove, it works.

@csmager csmager force-pushed the github-releases branch 2 times, most recently from d0268b1 to e69cb72 Compare October 26, 2017 15:53
@matthid
Copy link
Member

matthid commented Oct 28, 2017

Actually we have two command lines:

  1. The old full framework based (legacy FAKE.exe)
  2. The new netcore based (FAKE 5)

The command lines differ. I failed at not noticing that -pd in the old command line needs to be before --fsiargs as everything after it will not be noticed by FAKE and given to fsi.

Thanks for taking care of this. Your change (ie removing -pd) is fine as well.

@matthid matthid mentioned this pull request Oct 28, 2017
@matthid matthid merged commit 38fe9c5 into fsprojects:master Oct 28, 2017
@csmager csmager deleted the github-releases branch December 5, 2017 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants