Follow-up from "Only spins for UX reviewer if that team includes a designer"
The following discussions from !212 (merged) should be addressed:
-
@godfat-gitlab started a discussion: (+10 comments) @mvanremmerden I think I got this now. Combinations with different labels are tested at gitlab-org/gitlab!123816 (merged)
Please review, and feel free to play around with the testing merge request, by adding/removing labels and re-run the Danger job.
When you're happy with it, please pass to another reviewer/maintainer for this, because I am on vacation next week.
Note to the future reviewer/maintainer: I rushed this a bit therefore I didn't write tests for methods which I pulled from
Dangerfile
tolib/danger/plugins/roulette.rb
. My justifications for this are:- Sorry I ran out of time! But iterations, right?
😅 - If it's written in
Dangerfile
, there's no tests for them anyway... And it's making it very difficult to reuse code between simple roulette and the one in the GitLab project. See gitlab-org/gitlab!123816 (merged) . I think we really need to merge the rouletteDangerfile
because we're clearly duplicating code. Moving some of the code toroulette.rb
might be an even better idea so that it's easier to write tests as well, given that this is no longer simple at all!
- Sorry I ran out of time! But iterations, right?
-
@rymai started a discussion: suggestion: I don't think this comment is relevant in this method anymore. And it's also not super helpful in
#prepare_categories
as it's merely paraphrasing the Ruby code that's simple to understand. -
@rymai started a discussion: suggestion: Can we reduce the indirection here?
# We want the designer for the team to review the wider community # contribution because they're more familiar with that area. the_designer_for_the_team?(teammate) else # We don't want the designer for the team to review merge # requests for the same team which is designed by themselves. # So they can only review if they're not the designer for the team. !the_designer_for_the_team?(teammate)
-
@rymai started a discussion: -
@rymai started a discussion: observation: I'm very confused by this. The method name seems to suggest that we're looking for a designer to act as fallback for wider community contributions, but the implementation actually looks for a "spin" for UX where there's no reviewer nor maintainer.
In other terms, we're checking if:
- We have something to review UX-related (i.e.
spin.category == :ux
) - The roulette couldn't find anyone to assign for this category
But:
- We already check for
if categories.include?(:ux)
inlib/danger/rules/simple_roulette/Dangerfile
- We usually assign these "fallback" people directly in
#spin
, e.g.
when :test spin.optional_role = :maintainer if spin.reviewer.nil? # Fetch an already picked backend reviewer, or pick one otherwise spin.reviewer = backend_spin&.reviewer || spin_for_category(project, :backend, timezone_experiment: including_timezone).reviewer end
I'll make a suggestion in
#spin
and I think we can get rid of this method? - We have something to review UX-related (i.e.
-
@rymai started a discussion: spin.optional_role = :maintainer spin.reviewer = teammate_pedroms if labels.include?("Community contribution") && spin.reviewer.nil? && spin.maintainer.nil?
-
@rymai started a discussion: suggestion: We shouldn't need this method.
def assign_pedroms_for_ux_wider_community_contribution(spins) # We want at least a UX reviewer who can review any wider community # contribution even without a team designer. We assign this to Pedro. ux_spin = look_for_fallback_designer_for_ux_wider_community_contribution?(spins) ux_spin && ux_spin.reviewer = teammate_pedroms end
-
@rymai started a discussion: suggestion: I think we can avoid this entirely.