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 GitHub integration #839

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

alexmarucci
Copy link

Following up on the original PR #623

  • Added github_blame and github formatter config
  • Added PR info in line blame
  • Added PR number in line blame popup
  • If no PR is found, falls back to the std git blame

@alexmarucci
Copy link
Author

Hi @lewis6991, I hope PRs are welcomed. This is a feature I wanted for a while now, so I thought giving it a go.

Let me know your thoughts, also I might need a hand with the failing tests. Unsure why they're failing to be fair.

Thank you

@@ -75,9 +75,11 @@ require('gitsigns').setup {
virt_text = true,
virt_text_pos = 'eol', -- 'eol' | 'overlay' | 'right_align'
delay = 1000,
github_blame = false,
Copy link
Author

Choose a reason for hiding this comment

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

@lewis6991 should this be called something like use_github or something like that instead?

• `<number>`
• `<author>`
• `<mergedAt>` or `<mergedAt:FORMAT>`
• `<title>`
Copy link
Author

Choose a reason for hiding this comment

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

As an improvement, we could expand this to the full list of fields that gh gives us instead? only nested fields are not supported yet

if last_pr then
result = last_pr;
-- the parser does not support accessing keys like <author.name>
result.author = last_pr.author.name
Copy link
Author

Choose a reason for hiding this comment

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

<author.name> maybe that's something we could add? so we could just query the GH endpoint and pass down all the available fields

@lewis6991
Copy link
Owner

lewis6991 commented Jul 16, 2023

Hi. PRs are definitely welcome, however I have a relatively high bar for new features (just to be warned).

I'll have a fair amount of feedback to give, but I'll need to test this first.

Like everyone, I'm pretty busy, spread quite thin across several projects and only limited time to work on open source stuff so it might be a while until I can look at this properly.

One thing I'll say now is that this feature must not affect the experience of the plugin on non-github repos and therefore should purely be additive.

I'm also not so sure about adding another formatter field to the config. The formatter (string version) was added without the consideration of optional fields, whereas a single function can handle all cases.

The plugin must also not try to call gh if it is not installed, not just simply ignore the error.

Other than that, this looks like a good start. 👍

@alexmarucci
Copy link
Author

Hi. PRs are definitely welcome, however I have a relatively high bar for new features (just to be warned).

I'll have a fair amount of feedback to give, but I'll need to test this first.

Like everyone, I'm pretty busy, spread quite thin across several projects and only limited time to work on open source stuff so it might be a while until I can look at this properly.

One thing I'll say now is that this feature must not affect the experience of the plugin on non-github repos and therefore should purely be additive.

I'm also not so sure about adding another formatter field to the config. The formatter (string version) was added without the consideration of optional fields, whereas a single function can handle all cases.

The plugin must also not try to call gh if it is not installed, not just simply ignore the error.

Other than that, this looks like a good start. 👍

Nice, thank you for your feedback. That's totally fine, and I think I got enough to make few adjustments already 👍 I will see if I get some time to look into the failing tests.

In general, I see this as a starting point to make some progress on this feature following some directions from your side.

Speak soon 👋

@mechanicles

This comment was marked as spam.

@mechanicles

This comment was marked as resolved.

@lewis6991

This comment was marked as resolved.

@mechanicles

This comment was marked as resolved.

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.

3 participants