Added the required status code StatusCodes.CREATED for the createRele…

[bsutton]

Calls to createRelease were failing as the github api returns a 201 on a successful creation.

Added the required statusCode to the request and createRelease now completes as expected.

[bsutton]

Fixes: #230

createRelease was not working at all as it didn't register the expected return code of 201.

getReleaseByTagName failed if a 404 occurred. Now returns null.

Added asserts to RepositorySlug to ensure passed arguments were non-null.

[bsutton]

@robbecker-wf any chance of seeing this PR merged an an updated package pushed to pub.dev.

I'm currently block on releasing another package that depends on it.

[bsutton]

ping @robbecker-wf

[robrbecker]

Thanks for putting up this PR. I'll give it a review and some testing here soon.

[bsutton]

Thanks

…

On Fri, 4 Sep 2020, 8:05 am Rob Becker, ***@***.***> wrote: Thanks for putting up this PR. I'll give it a review and some testing here soon. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#231 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG32OETAVXMTNAPGWE74DDSEAHI5ANCNFSM4P6NSIIA> .

[robrbecker]

This part looks ok to me.

[robrbecker]

👍

[robrbecker]

👍

[bsutton]

I have a feeling that returning null from hasErrors was causing a problem which is why I changed it to return false. I can't quite remember the circumstances.

[bsutton]

I've just checked and the method createRelease has the following line:

if (release.hasErrors) {

As such hasErrors should never be null.
My experience with dart is that passing a bool with a null value always ends with someone crying.

[robrbecker]

If I'm reading this correctly, this would mean that if a release isn't found callers might just get a null and not really know why. The exception is handled internally and silenced.
If that's correct it would mean it behaves inconsistently with other methods. For example the getReleaseById method would throw an exception if the release doesn't exist whereas this method returns null.

Would you be open to simply allowing the ReleaseNotFound exception to come out of this function and handling the exception at the caller level in your consuming code?

[bsutton]

Consistency is probably the right answer.

Personally I don't think an exception should be thrown unless something 'exceptional' happens.

I would consider not finding a release a part of normal processing rather than an exceptional set of circumstances.

The resulting code is also easier to work with

if ((release = getReleaseByTagname(xxx)) != null)
{ 
   release.delete();
}

as apposed to

try
{
   var release = getReleaseByTagName(xxxx);
   release.delete();
}
on ReleaseNotFound catch(e)
{
/// no op
}

I will be guided by you as to which way you want it done.

[robrbecker]

I think I'd prefer the consistency with other methods and not return null. If someone did want to handle the case when a release is not found .. they wouldn't be able to. Here's what I was thinking it should look like:

  Future<Release> getReleaseByTagName(
      RepositorySlug slug, String tagName) async {
    return github.getJSON(
      '/repos/${slug.fullName}/releases/tags/$tagName',
      convert: (i) => Release.fromJson(i),
      statusCode: StatusCodes.OK,
      fail: (http.Response response) {
        if (response.statusCode == 404) {
          throw ReleaseNotFound.fromTagName(github, tagName);
        }
      },
    );
  }

[bsutton]

OK, I have modified the code as requested. I will run a couple of tests and push the code.

[robrbecker]

Done with my review pass, back to you @bsutton

[robrbecker]

Suggested change

// github.getJSON('/repos/${slug.fullName}/releases/tags/$tagName', convert: (i) => Release.fromJson(i));

[bsutton]

@robbecker-wf New PR with changes as requested.

I still hate that it throws an exception ;)

[robrbecker]

LGTM. Are you ready for it to be merged?

[bsutton]

Yes please

…

On Wed, 16 Sep 2020, 5:24 am Rob Becker, ***@***.***> wrote: ***@***.**** approved this pull request. LGTM. Are you ready for it to be merged? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#231 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG32OFVD2FA2MOJG5ESYITSF65OJANCNFSM4P6NSIIA> .

[bsutton]

thanks!