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.

Sign up for GitHub

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

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

Merged
merged 7 commits into from
Sep 15, 2020
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
34439bf
Added the required status code StatusCodes.CREATED for the createRele…
bsutton Aug 13, 2020
5d41ad0
null bool was returned when errors was null which was causing an asse…
bsutton Aug 13, 2020
5700cda
fixed formatting.
bsutton Aug 13, 2020
7ba7629
Added log to handle a 404 which occurs if the tag doesn't exist.
bsutton Aug 24, 2020
16bfe55
Added asserts to ensure that each argument to RepositorySlug is non-null
bsutton Aug 24, 2020
dfe06d7
Added new Exception for ReleaseNotFound.
bsutton Aug 24, 2020
6caf3ed
Modified getReleaseByTagName to thrown an exception if the release ca…
bsutton Sep 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lib/src/common/model/repos.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,10 @@ class RepositorySlug {
/// Repository Name
String name;

RepositorySlug(this.owner, this.name);
RepositorySlug(this.owner, this.name) {
ArgumentError.checkNotNull(owner, 'owner');
ArgumentError.checkNotNull(name, 'name');
}
Copy link
Member

Choose a reason for hiding this comment

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

👍


/// Creates a Repository Slug from a full name.
factory RepositorySlug.full(String f) {
Expand Down
2 changes: 1 addition & 1 deletion lib/src/common/model/repos_releases.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ class Release {
String getUploadUrlFor(String name, [String label]) =>
"${uploadUrl.substring(0, uploadUrl.indexOf('{'))}?name=$name${label != null ? ",$label" : ""}";

bool get hasErrors => errors?.isNotEmpty;
bool get hasErrors => errors == null ? false : errors.isNotEmpty;
Copy link
Member

Choose a reason for hiding this comment

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

👍

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

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

}

/// Model class for a release asset.
Expand Down
35 changes: 28 additions & 7 deletions lib/src/common/repos_service.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -968,10 +968,31 @@ class RepositoriesService extends Service {

/// Fetches a single release by the release tag name.
///
/// Returns null if the release doesn't exist.
///
/// API docs: https://developer.github.com/v3/repos/releases/#get-a-release-by-tag-name
Future<Release> getReleaseByTagName(RepositorySlug slug, String tagName) =>
github.getJSON('/repos/${slug.fullName}/releases/tags/$tagName',
convert: (i) => Release.fromJson(i));
Future<Release> getReleaseByTagName(
RepositorySlug slug, String tagName) async {
// github.getJSON('/repos/${slug.fullName}/releases/tags/$tagName', convert: (i) => Release.fromJson(i));
Copy link
Member

Choose a reason for hiding this comment

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

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


Release release;
try {
release = await github.getJSON(
'/repos/${slug.fullName}/releases/tags/$tagName',
convert: (i) => Release.fromJson(i),
statusCode: StatusCodes.OK,
fail: (http.Response response) {
if (response.statusCode == 404) {
// we just return null if the tag can't be found.
throw ReleaseNotFound.fromTagName(github, tagName);
}
},
);
} on ReleaseNotFound catch (e) {
release = null;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@bsutton bsutton Sep 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@robrbecker robrbecker Sep 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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);
        }
      },
    );
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
return release;
}

/// Creates a Release based on the specified [createRelease].
///
Expand All @@ -986,10 +1007,10 @@ class RepositoriesService extends Service {
ArgumentError.checkNotNull(slug);
ArgumentError.checkNotNull(createRelease);
final release = await github.postJSON<Map<String, dynamic>, Release>(
'/repos/${slug.fullName}/releases',
convert: (i) => Release.fromJson(i),
body: GitHubJson.encode(createRelease.toJson()),
);
'/repos/${slug.fullName}/releases',
convert: (i) => Release.fromJson(i),
body: GitHubJson.encode(createRelease.toJson()),
statusCode: StatusCodes.CREATED);
Copy link
Member

Choose a reason for hiding this comment

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

This part looks ok to me.

if (release.hasErrors) {
final alreadyExistsErrorCode = release.errors.firstWhere(
(error) => error['code'] == 'already_exists',
Expand Down
6 changes: 6 additions & 0 deletions lib/src/common/util/errors.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ class RepositoryNotFound extends NotFound {
: super(github, 'Repository Not Found: $repo');
}

/// Release not found
class ReleaseNotFound extends NotFound {
const ReleaseNotFound.fromTagName(GitHub github, String tagName)
: super(github, 'Release for tagName $tagName Not Found.');
}

/// GitHub User was not found
class UserNotFound extends NotFound {
const UserNotFound(GitHub github, String user)
Expand Down