Skip to content

Fix & improve github.createTag - #15

Merged
hypherionmc merged 1 commit into
firstdarkdev:2.0from
MattSturgeon:fix/createTag
Apr 24, 2024
Merged

Fix & improve github.createTag#15
hypherionmc merged 1 commit into
firstdarkdev:2.0from
MattSturgeon:fix/createTag

Conversation

@MattSturgeon

@MattSturgeon MattSturgeon commented Apr 4, 2024

Copy link
Copy Markdown
Contributor

Fixed the inverted logic and avoided the need to fetch all repo tags by using getRef instead.

I tested out the hasRef implementation as follows:

Details

// Some boilerplate setting up github, token & repo omitted
fun main() {
    sequenceOf("v1.2.3", "foo", "random", "v1.2.2", "v1.2.3+mc1.20.4")
        .forEach {
            println("Checking if ${repo.fullName} has tag named $it")
            val has = repo.hasTag(it)
            println("  It ${if (has) "does" else "does not"}!")
        }

}

fun GHRepository.hasTag(tag: String): Boolean {
    try {
        this.getRef("refs/tags/$tag")
        return true
    } catch (e: GHFileNotFoundException) {
        return false
    } catch (e: IOException) {
        throw IOException("Error checking remote tag", e)
    }
}

Which prints:

Checking if MinecraftFreecam/Freecam has tag named v1.2.3
  It does!
Checking if MinecraftFreecam/Freecam has tag named foo
  It does not!
Checking if MinecraftFreecam/Freecam has tag named random
  It does not!
Checking if MinecraftFreecam/Freecam has tag named v1.2.2
  It does!
Checking if MinecraftFreecam/Freecam has tag named v1.2.3+mc1.20.4
  It does!

Fixes #12
Fixes #14

- Check was inverted (firstdarkdev#14)
- Check was inefficient (firstdarkdev#12)
Comment on lines +112 to +113
// FIXME this check doesn't make sense when `github.draft` is true,
// because draft releases don't create a tag anyway...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This API was broken and is marked "experimental". Should we take this opportunity to change behaviour for the creating a draft scenario?

What do you think the best approach is?

  • rename the option for clarity?
  • just update the docs?
  • add draft/public specific options?
  • take a string/enum instead of a boolean?

@hypherionmc

Copy link
Copy Markdown
Member

Hey. Just checking in. I've been so busy I forgot to check back on things.

Is this PR ready to be merged, or do you have more you want to do first?

@MattSturgeon

Copy link
Copy Markdown
Contributor Author

I've been so busy I forgot to check back on things.

No worries 👍

Is this PR ready to be merged

Should be good to go, unless you request any changes.

do you have more you want to do first?

It doesn't address this comment or any of the ideas discussed in #16, but it does fix actual issues in the current implementation.

Those other changes are out of scope of this PR, I think.

@hypherionmc
hypherionmc merged commit a4b2b41 into firstdarkdev:2.0 Apr 24, 2024
@MattSturgeon
MattSturgeon deleted the fix/createTag branch April 24, 2024 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] github.createTag is inverted Check if tag exists without getting all tags

2 participants