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

Add issue completion #2513

Merged
merged 2 commits into from
Aug 20, 2020
Merged

Add issue completion #2513

merged 2 commits into from
Aug 20, 2020

Conversation

onukura
Copy link
Member

@onukura onukura commented Aug 16, 2020

Before submitting a pull-request to GitBucket I have first:

  • read the contribution guidelines
  • rebased my branch over master
  • verified that project is compiling
  • verified that tests are passing
  • squashed my commits as appropriate (keep several commits if it is relevant to understand the PR)
  • marked as closed using commit message all issue ID that this PR should correct

This PR add issue completion to resolve #568 .

And it includes s change in base trait SuggestionProvider.

  • add arg RepositoryInfo to additionalScript method to generate dynamic url path.

Screen gif

screenshot3

Copy link
Member

@SIkebe SIkebe left a comment

Choose a reason for hiding this comment

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

Really excited about this! 😄

@@ -31,6 +31,9 @@ trait IssuesService {
Issues filter (_.byPrimaryKey(owner, repository, issueId.toInt)) firstOption
else None

def getAllIssues(owner: String, repository: String)(implicit s: Session): List[Issue] =
Issues filter (_.byRepository(owner, repository)) sortBy (_.issueId desc) list
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more user-friendly if only open issues/PRs are visible, just like milestone filtering.
In many cases, there are hundreds of closed issues/PRs in a long-living repository.

image

Copy link
Member Author

Choose a reason for hiding this comment

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

@SIkebe Thx for your review! Showing all Issues and PR is because of GitHub. They actually show everything. And I thought it make sense because jquery completion show only 10 issues and PR at once and ordered by issue id so from new to old. In many case new id is still open. And by contrary it could be inconvenient if closed issue can not be show up. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or maybe we can change color by status (open or close)...

Copy link
Member

Choose a reason for hiding this comment

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

Showing all Issues and PR is because of GitHub. They actually show everything

GitHub shows only open issues/PRs. GitLab also does the same thing. In my experience, you refer an open issue more frequently than closed one.

image

Copy link
Member

Choose a reason for hiding this comment

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

GitLab


image

Copy link
Member

Choose a reason for hiding this comment

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

I also agree with showing only open issues/PR as completion proposals.

Copy link
Member Author

Choose a reason for hiding this comment

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

@SIkebe @takezoe
Ok! So I will fix PR to show only Opened issues/PRs.
About this method ( getAllIssues ), I think it might be better preserved because it's more general than getting only opened isssues/PRs.
And filter at IssueController like fllowing code.

What do you think?

          getAllIssues(repository.owner, repository.name)
            .filterNot(_.closed)
            .map { t =>

Copy link
Member

Choose a reason for hiding this comment

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

In terms of performance, it would be better to narrow down at the SQL layer. You can implement getAllIssues any time you really need it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. It would be slow to get all... ok! So I will fix sql layer! I mean this method!

Copy link
Member Author

Choose a reason for hiding this comment

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

@takezoe @SIkebe I fixed method.

@SIkebe SIkebe added the feature label Aug 17, 2020
@takezoe takezoe added this to the 4.35.0 milestone Aug 20, 2020
Copy link
Member

@takezoe takezoe left a comment

Choose a reason for hiding this comment

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

This looks good to me!

Copy link
Member

@SIkebe SIkebe left a comment

Choose a reason for hiding this comment

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

Cool!

@takezoe takezoe merged commit 8db98d7 into gitbucket:master Aug 20, 2020
@onukura onukura deleted the issue_completion branch August 27, 2020 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

autocompletion for # or @ in issues
3 participants