Skip to content

Conversation

@Catsuko
Copy link
Contributor

@Catsuko Catsuko commented Mar 11, 2025

This PR reworks the OffensesFormatter#format_offenses method to accept a Danger::Plugin instance rather than repo url.

Why

In the default formatter, linking to offending constants assumes a main branch does not work in repos that do not use a main branch. Linking to the "main" branch also does not work well in situations where the constant does not exist or has been moved\changed in the PR.

Danger provides has a rich set of formatting tools which I think are useful here and for custom formatters in general: https://danger.systems/reference. This includes the github#html_link method which is made for this kind of linking.

Problems

  • #html_link provides an anchor tag which does not work well with the existing markdown formatting
  • breaking change for existing OffensesFormatter classes that people are using

I'm interested to know what people think about this 🙏

).returns(String)
end
def format_offenses(offenses, repo_link, org_name)
def format_offenses(offenses, plugin, org_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for making this PR! I think this is a great idea because as you pointed out, not every project is going to have a branch called main.

To make things more backwards compatible here and minimize changes, we could continue to pass in a repo_link but extend it to include the branch (#{github.pr_json[:base][:repo][:html_url]}/blob/#{github.head_commit} instead of github.pr_json[:base][:repo][:html_url]). This would require less refactoring so old uses of the previous type signature wouldn't result in exceptions.

Copy link
Contributor Author

@Catsuko Catsuko Mar 17, 2025

Choose a reason for hiding this comment

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

To make things more backwards compatible here and minimize changes, we could continue to pass in a repo_link but extend it to include the branch (#{github.pr_json[:base][:repo][:html_url]}/blob/#{github.head_commit} instead of github.pr_json[:base][:repo][:html_url]). This would require less refactoring so old uses of the previous type signature wouldn't result in exceptions.

I like the idea of making it more backwards compatible but i think including the branch would be a breaking change for any formatter that doesn't expect repo_link to include the branch. For example, take the existing default implementation:

[<ins>`#{constant_name}`</ins>](#{repo_link}/blob/main/#{constant_location})

Do you think that would be a problem?

@martinemde
Copy link
Contributor

@Catsuko, would #54 work for your use case or do you need the features provided by the full plugin interface?

@Catsuko
Copy link
Contributor Author

Catsuko commented Mar 24, 2025

@martinemde
That would work for me 🙌

I think the plugin interface is pretty nice but all i really want to do is replace main with the head commit of the PR.

@martinemde
Copy link
Contributor

Yeah, I think we could reassess if there were more needs for the full plugin interface.

@Catsuko
Copy link
Contributor Author

Catsuko commented Mar 25, 2025

@martinemde
In that case, i'll close my PR and await your changes. Thanks for the help!

@Catsuko Catsuko closed this Mar 25, 2025
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