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

closing-remarks shows earliest tag alphabetically, not chronologically #7208

Open
aryairani opened this issue Jan 9, 2024 · 14 comments
Open

Comments

@aryairani
Copy link

Description

The feature says the first tag containing the PR was pre-release

image

But the first tag is actually release/0.5.13; pre-release happens to be the last tag containing the PR.

image

Note: the pre-release tag in this repo changes over time, but the bug still happens after clearing the extension cache, so I don't suppose it's a stale result.

How to replicate the issue + URL

unisonweb/unison#4519

Extension version

23.12.17

Browser(s) used

Chrome 120.0.6099.129 (Official Build) (arm64)

@aryairani aryairani added the bug label Jan 9, 2024
@fregante
Copy link
Member

fregante commented Jan 9, 2024

@fregante fregante marked this as a duplicate of #7206 Jan 9, 2024
@fregante fregante closed this as not planned Won't fix, can't repro, duplicate, stale Jan 9, 2024
@fregante fregante changed the title "Shows the first Git tag a merged PR was included in" shows me the wrong tag? closing-remarks shows pre-release tags Jan 9, 2024
@fregante
Copy link
Member

fregante commented Jan 9, 2024

Thank you @aryairani 🙏 ☺️

@aryairani
Copy link
Author

Hi @fregante — I'm a little uncertain. I don't believe it's an issue of excluding or including pre-release tags.

My understanding is that the UI is supposed to show the earliest tag that included the PR in question. In my scenario, the earliest tag isn't the pre-release tag, it's the actual release tag; but the UI is showing the pre-release tag.

What am I missing?
Thank you!

@aryairani
Copy link
Author

aryairani commented Jan 9, 2024

(Actually I think it might the same issue described in #7206, but the submitter mistakenly focused on "pre-release" vs "release" metadata, which I think are a red herring which led to the issue being closed prima facie.)

@fregante
Copy link
Member

We just ask GitHub for the order, so you can change it by changing the time of creation of the tags. But the core logic remains: this pattern requires --force, so it's not something we should support

@aryairani aryairani changed the title closing-remarks shows pre-release tags closing-remarks doesn't show earliest tag Jan 10, 2024
@aryairani
Copy link
Author

aryairani commented Jan 10, 2024

We just ask GitHub for the order, so you can change it by changing the time of creation of the tags. But the core logic remains: this pattern requires --force, so it's not something we should support

Sorry, I still didn't understand. I do want pre-release tags to be included in the results. #7206 is asking to exclude pre-releases, but I'm not asking for that. I'm just reporting that in this case the pre-release tag is not the earliest tag, but it's being reported as the earliest tag.

I think maybe the order is coming back wrong or needs to be sorted differently?

image image

release/0.5.13 includes the PR and is earlier than pre-release

We just ask GitHub for the order

Where does this happen? Maybe I can help debug.

@fregante
Copy link
Member

See the firstTag updater function: https://github.com/refined-github/refined-github/blob/main/source/features/closing-remarks.tsx

I wonder if GitHub is showing the tags in an unexpected order in the dom, I thought we were using the API. Probably there isn't an equivalent API.

@aryairani
Copy link
Author

Should we reopen this bug since I don't think it's a dup of #7206?

@fregante
Copy link
Member

So it seems that GitHub lists tag in alphabetical order:

We expect the "earliest tag" to appear last on that list, because that's generally the case.

To fix this issue and to future-proof this feature, I'd be open to use the API, if it's possible. I think we went with this snippet because the API doesn't expose this information, calculating it would be expensive.

@fregante fregante reopened this Jan 10, 2024
@fregante fregante changed the title closing-remarks doesn't show earliest tag closing-remarks doesn't show earliest tag if non-version tags exist Jan 10, 2024
@aryairani
Copy link
Author

aryairani commented Jan 10, 2024

As a workaround I will maybe rename my pre-release to nightly, which comes before release in alphabetical order. 🤦 Thanks for looking into this.

@wookayin
Copy link

wookayin commented Jan 10, 2024

@aryairani Alphabetically, nightly < pre-release < release-* < v, so you might instead want a tag that comes after release, i.e. s, t, u, v, w, ... I have a similar issue with you in #7206, where nightly precedes v.

@aryairani
Copy link
Author

aryairani commented Jan 12, 2024

@wookayin Oh yeah! 🤦
@fregante maybe this issue should be "closing-remarks shows earliest tag alphabetically rather than chronologically".
i.e. you could see the same problem if the versioning scheme changes (even though none of them would necessarily be non-version tags)

@aryairani aryairani changed the title closing-remarks doesn't show earliest tag if non-version tags exist closing-remarks shows earliest tag alphabetically, not chronologically Jan 26, 2024
@aryairani
Copy link
Author

Ok, I guess I have my functioning workaround in place now, thanks to @wookayin's alphabet help, by calling my nightly tag trunk-build, which comes after release/* alphabetically. (I had called it my "trunk build" tag trunk briefly, but ran into conflicts with the trunk branch.)

@fregante
Copy link
Member

fregante commented Feb 2, 2024

Tags that include the word nightly and tags that don't have a version (minimum \d\.\d expected) are now excluded:

This specific issue can stay open because I'd still want ordering, but I think this can only be done via API. The API can return the tags in order of creation.

I don't want to get into the sorting of tags out of the current HTML snippet because soon enough there will be a repo with 1.0.0 and release/2.0.0 and beta/2.0.0 and I can't figure out every combination of that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants