Do not update to latest version if it is prerelase

[triallax]

Fixes #10.

[triallax]

I'm not sure how to check that my changes are actually working correctly. Also, the CI is failing for some reason.

[triallax]

I also formatted the code using purty because I was too lazy to format it manually.

[thomashoneyman]

@mhmdanas I'll have some time to come back to this tomorrow for a proper review. In the meantime, would you mind not formatting with Purty? It's much easier to review if I can see just what's changed (vs. the whole file being changed). Any formatting should ideally happen in a separate PR.

That said, due to some formatting issues with Purty I'm not quite ready to switch to it across these projects. I hope to someday, but if you could try and keep the formatting roughly the same as what exists already I would really appreciate that.

[triallax]

I have no problem with that. Will try to revert the formatting as much as possible.

[triallax]

I reverted all the formatting of the already existing code. I should probably ask if I can format the code before I actually do it. I'll investigate why the CI is failing.

[triallax]

I dug a bit into the generated JS, using good ol' console.logs along the way (should I learn how to use a debugger?), and found something interesting. The response of this request is a Not found error:

Right {
  value0: {
    body: {
      message: 'Not Found',
      documentation_url: 'https://docs.github.com/rest'
    },
    headers: [
      [ResponseHeader], [ResponseHeader],
      [ResponseHeader], [ResponseHeader],
      [ResponseHeader], [ResponseHeader],
      [ResponseHeader], [ResponseHeader],
      [ResponseHeader], [ResponseHeader],
      [ResponseHeader], [ResponseHeader],
      [ResponseHeader], [ResponseHeader],
      [ResponseHeader], [ResponseHeader],
      [ResponseHeader], [ResponseHeader],
      [ResponseHeader], [ResponseHeader]
    ],
    status: 404,
    statusText: 'Not Found'
  }
}

[triallax]

It seems I fixed the problem. I was already suspicious that the slash before the question mark was the cause. Is this the new "I forgot to add a semicolon" but reversed?

Edit: seems like there's a version that doesn't comply with SemVer. What should I do?

[thomashoneyman]

seems like there's a version that doesn't comply with SemVer. What should I do?

We should only update to the latest version if it is a) a SemVer version and b) not a pre-release version.

I'll investigate why the CI is failing.

No need! It's failing because of an issue with the FFI in the actions toolkit bindings, and that's fixed and released. I'll update this repository to use that release.

[triallax]

How do we know if it's actually a version, even if it's not SemVer? It could be a non-SemVer version, or it might not even be a version. Should I not care about this and just ignore any invalid versions?

[thomashoneyman]

Yea, anything invalid should be ignored. We only want the latest valid SemVer, non-prerelease version. In fact, I saw in purescript-contrib/governance#25 that draft releases are also included in the GitHub response, so we'll want to filter those out too. Jordan wrote some similar code in that PR to what you're writing here, which may be helpful!

[triallax]

I see. Should I log a warning about invalid versions?

For the PR you linked to, Jordan's code does look similar to mine. I'll look in it if I need anything.

Also, for the default number of entries per page, I used 10, but I think 5 might be enough. Should I keep it as it is or change it?

Edit: also, what are draft releases exactly? First time I heard of them.

[thomashoneyman]

Should I log a warning about invalid versions?

I think that would be a nice addition! In the github-actions-toolkit bindings there's a function for producing warnings (I believe it's actually called warning).

Also, for the default number of entries per page, I used 10, but I think 5 might be enough. Should I keep it as it is or change it?

I think 10 entries is fine. We could even pull the maximum, which I believe is 30. It's better to bring in the larger response so that we have to make fewer requests in the worst case. I think the difference between pulling 5 and 30 results will be negligible.

what are draft releases exactly? First time I heard of them.

When you publish a new release on GitHub -- not a tag, but the release itself -- you can choose to make it a "draft" to indicate it is not actually published yet. A draft release won't be listed as the "latest" release. It may not even be visible in the UI except to project collaborators, I'm not sure. In any case we definitely don't want to pull in draft releases and set that in CI!

[thomashoneyman]

By the way, as of #13 the CI should be fixed. You can merge that into this branch to get builds working again.

[triallax]

I think that would be a nice addition! In the github-actions-toolkit bindings there's a function for producing warnings (I believe it's actually called warning).

Thanks! I'll try using that.

I think 10 entries is fine. We could even pull the maximum, which I believe is 30. It's better to bring in the larger response so that we have to make fewer requests in the worst case. I think the difference between pulling 5 and 30 results will be negligible.

The maximum is actually 100. I asked if I should make it 5 because currently the tools mostly don't use pre-releases. But you're right in that the difference is negligible. I personally think we should keep it at 10, but if you think otherwise I'm fine with it.

When you publish a new release on GitHub -- not a tag, but the release itself -- you can choose to make it a "draft" to indicate it is not actually published yet. A draft release won't be listed as the "latest" release. It may not even be visible in the UI except to project collaborators, I'm not sure. In any case we definitely don't want to pull in draft releases and set that in CI!

I'll see how I can get that from the JSON response.

[triallax]

No need! It's failing because of an issue with the FFI in the actions toolkit bindings, and that's fixed and released. I'll update this repository to use that release.

Actually, as you can see in one of my comments, I had an extraneous slash which caused GitHub to return a "Not Found" error. So it's also on me!

[thomashoneyman]

I personally think we should keep it at 10, but if you think otherwise I'm fine with it.

Fine by me!

I'll see how I can get that from the JSON response.

You can see a JSON codec that Jordan implemented in his PR that I linked. It includes that field.

[thomashoneyman]

Thanks!