Skip to content

Include other bug labels in bugs-tab #4592

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

Jump to bottom
Merged
fregante merged 33 commits into from
Jul 28, 2021
Merged

Include other bug labels in bugs-tab #4592

fregante merged 33 commits into from
Jul 28, 2021

Conversation

@yakov116
Copy link
Member

@yakov116 yakov116 commented Jul 20, 2021 •
edited
Loading

@yakov116 yakov116 changed the title First atempt Include other bug labels in bugs-tab Jul 20, 2021
@fregante
Copy link
Member

fregante commented Jul 20, 2021

gif

@yakov116
Copy link
Member Author

yakov116 commented Jul 20, 2021 •
edited
Loading

OMG!! WOW How did I make such a mistake 🤦

fregante
fregante reviewed Jul 20, 2021
View reviewed changes
yakov116
yakov116 commented Jul 20, 2021
View reviewed changes
@fregante fregante force-pushed the main branch 2 times, most recently from 461391d to da213cf Compare July 20, 2021 15:33
fregante
fregante reviewed Jul 20, 2021
View reviewed changes
@fregante
Copy link
Member

fregante commented Jul 20, 2021

This feature keeps getting longer and longer…. 😅

@yakov116
Copy link
Member Author

yakov116 commented Jul 20, 2021

Buggy code 😏

@yakov116 yakov116 closed this Jul 20, 2021
@yakov116 yakov116 reopened this Jul 20, 2021
@yakov116 yakov116 marked this pull request as ready for review July 21, 2021 03:13
@yakov116 yakov116 requested a review from fregante July 21, 2021 03:14
fregante
fregante reviewed Jul 21, 2021
View reviewed changes
fregante
fregante reviewed Jul 21, 2021
View reviewed changes
yakov116
yakov116 commented Jul 21, 2021
View reviewed changes
@yakov116
Copy link
Member Author

yakov116 commented Jul 26, 2021

I'll look into it when I have time. But yet maybe you can revert the change where you're having trouble

@fregante ready.

Right now the only thing that is broken is the .add it needs "

fregante
fregante requested changes Jul 26, 2021
View reviewed changes
fregante
fregante reviewed Jul 27, 2021
View reviewed changes
yakov116
yakov116 commented Jul 27, 2021
View reviewed changes
source/github-helpers/search-query.ts Outdated
}

static escapeValue(value: string): string {
return /\s|^:/.test(value) ? `"${value}"` : value;
Copy link
Member Author

@yakov116 yakov116 Jul 27, 2021

Choose a reason for hiding this comment

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

Changed is since GitHub will add quotes if it starts with a : or if it contains a space you can test it https://github.com/yakov116/TestR/labels

Copy link
Member

@fregante fregante Jul 27, 2021

Choose a reason for hiding this comment

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

Doesn't look accurate: https://github.com/fregante/sandbox/issues?q=is%3Aopen+sort%3Aupdated-desc+label%3A%3Aa%3A

I think it only happens for spaces, but there might be other characters as well. It might be worth creating new labels to test this out. You can do so in a test repo from the issue sidebar

Copy link
Member Author

@yakov116 yakov116 Jul 27, 2021

Choose a reason for hiding this comment

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

I did you want more tests?

Copy link
Member

@fregante fregante Jul 27, 2021

Choose a reason for hiding this comment

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

I mean manual tests on GitHub.com to determine what characters triggers the quotes. From what I see, currently this should be:

Suggested change
return /\s|^:/.test(value) ? `"${value}"` : value;
return value.includes(' ') ? `"${value}"` : value;

Copy link
Member Author

@yakov116 yakov116 Jul 27, 2021

Choose a reason for hiding this comment

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

But what about

https://github.com/yakov116/TestR/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+label%3A%22t+%3A%2B1%3A%22 ??

Copy link
Member Author

@yakov116 yakov116 Jul 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

maybe if it has an emoji?

Copy link
Member Author

@yakov116 yakov116 Jul 27, 2021

Choose a reason for hiding this comment

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

Great idea lets include!

https://github.com/markdown-it/markdown-it-emoji/blob/339abf72f84a02e66fe9a4d682a7217d1f660e6a/dist/markdown-it-emoji.js#L9-L1848

Copy link
Member

@fregante fregante Jul 28, 2021

Choose a reason for hiding this comment

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

… that label has a space

Copy link
Member

@fregante fregante Jul 28, 2021

Choose a reason for hiding this comment

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

Also emojis are fine: https://github.com/fregante/sandbox/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+label%3A😃m

@fregante fregante merged commit f09a145 into main Jul 28, 2021
@fregante fregante deleted the bug-me branch July 28, 2021 06:27
@fregante
Copy link
Member

fregante commented Aug 2, 2021

Uhhhh this could have been useful 2 weeks ago 😂 https://github.blog/changelog/2021-08-02-search-issues-by-label-using-logical-or/

Might be worth updating the feature? The current situation is still better for the user though since:

  • only one label appears in the query
  • we can support any label with regex

So we're good without it anyway 😁

@yakov116
Copy link
Member Author

yakov116 commented Aug 3, 2021 •
edited
Loading

Yup I think the way it is now is good. But if we see a need we know we have other options.

@omBratteng omBratteng mentioned this pull request Aug 10, 2021
Merged
@mcornella
Copy link
Contributor

mcornella commented Sep 23, 2021

This doesn't seem to support uppercase characters in the label for me. Shouldn't the regex end with /i?

@yakov116
Copy link
Member Author

yakov116 commented Sep 23, 2021

This doesn't seem to support uppercase characters in the label for me. Shouldn't the regex end with /i?

Can you give a real url?

@mcornella
Copy link
Contributor

mcornella commented Sep 23, 2021

This one from the original post doesn't seem to work for me:

Captura de pantalla de 2021-09-23 19-14-46

But this repo works:

Captura de pantalla de 2021-09-23 19-15-30

@yakov116
Copy link
Member Author

yakov116 commented Sep 23, 2021

Thanks
Can you open a new issue?

@mcornella
Copy link
Contributor

mcornella commented Sep 23, 2021

I've opened #4811. I tried the /i in supportedLabels and it looks like it solves it.

@yakov116
Copy link
Member Author

yakov116 commented Sep 23, 2021

You can send a PR

@fregante fregante mentioned this pull request Sep 24, 2021
Closed
1 task
@yakov116 yakov116 mentioned this pull request Jul 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fregante fregante fregante requested changes

Assignees

No one assigned

Labels

enhancement

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support for labels other than "bug" in bugs-tab

4 participants

@yakov116 @fregante @Milo123459 @mcornella