Skip to content

Conversation

donat
Copy link
Member

@donat donat commented Dec 16, 2024

This is a PR to review changes at #31674 in reviewable steps.

Reviewing cheatsheet

Before merging the PR, comments starting with

  • ❌ ❓must be fixed
  • 🤔 💅 should be fixed
  • 💭 may be fixed
  • 🎉 celebrate happy things

@donat donat marked this pull request as ready for review December 16, 2024 12:34
@donat donat requested review from a team as code owners December 16, 2024 12:34
@donat donat requested a review from a team December 16, 2024 12:34
@donat donat requested review from a team as code owners December 16, 2024 12:34
@donat donat requested a review from a team December 16, 2024 12:34
@donat donat requested review from a team as code owners December 16, 2024 12:34
@donat donat requested review from a team, abstratt, big-guy, mlopatkin and reinsch82 and removed request for a team, abstratt, big-guy and mlopatkin December 16, 2024 12:34
@donat

This comment has been minimized.

@bot-gradle

This comment has been minimized.

Copy link
Member

@reinsch82 reinsch82 left a comment

Choose a reason for hiding this comment

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

one question

* @return the instance
* @since 8.13
*/
public static IdFactory instance() {
Copy link
Member

Choose a reason for hiding this comment

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

💭 Should this be a service?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the interesting bit in the review. My stance is that problem Ids and groups are lightweight descriptors that Gradle and third party plugins both should be able to define as static data. ie:

class MyProblemIds {
    public static final ProblemGroup  group = ...
    public static final ProblemId id1 = ...
    public static final ProblemId id2 = ...
}

I played around with making it a service but it made the the whole Problems API usage a fair bit clunky to use.

@bot-gradle

This comment has been minimized.

@bot-gradle

This comment has been minimized.

@bot-gradle

This comment has been minimized.

@bot-gradle

This comment has been minimized.

@donat donat force-pushed the donat/problems/stabilization-step1 branch from 641b659 to 149def4 Compare December 17, 2024 15:36
@donat

This comment has been minimized.

@bot-gradle

This comment has been minimized.

@donat donat force-pushed the donat/problems/stabilization-step1 branch from 149def4 to d0dfd7e Compare December 17, 2024 16:14
@donat

This comment has been minimized.

@bot-gradle

This comment has been minimized.

@bot-gradle

This comment has been minimized.

@bot-gradle

This comment has been minimized.

@donat donat force-pushed the donat/problems/stabilization-step1 branch from d0dfd7e to 28ced5e Compare December 18, 2024 08:33
@donat

This comment has been minimized.

@bot-gradle

This comment has been minimized.

@donat donat force-pushed the donat/problems/stabilization-step1 branch from 28ced5e to ef3c29e Compare December 18, 2024 08:37
@donat

This comment has been minimized.

@bot-gradle

This comment has been minimized.

@bot-gradle
Copy link
Collaborator

@bot-gradle
Copy link
Collaborator

@donat donat force-pushed the donat/problems/stabilization-step1 branch from ef3c29e to 60279d3 Compare December 18, 2024 11:39
@donat
Copy link
Member Author

donat commented Dec 18, 2024

@bot-gradle merge

@bot-gradle bot-gradle added this pull request to the merge queue Dec 18, 2024
@bot-gradle bot-gradle added this to the 8.13 RC1 milestone Dec 18, 2024
Merged via the queue into master with commit 7e90891 Dec 18, 2024
22 checks passed
@bot-gradle bot-gradle deleted the donat/problems/stabilization-step1 branch December 18, 2024 12:48
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.

4 participants