Skip to content

Comments

Fix #65 - the Dlang-Bot should use the 'Enhancement' label#84

Merged
PetarKirov merged 1 commit intodlang:masterfrom
wilzbach:fix-65
Jun 25, 2017
Merged

Fix #65 - the Dlang-Bot should use the 'Enhancement' label#84
PetarKirov merged 1 commit intodlang:masterfrom
wilzbach:fix-65

Conversation

@wilzbach
Copy link
Contributor

Rather trivial.

At this point we don't know about the existing labels, even though we have requested it before.
I will change our GH API in another PR to use internal caching and thus avoid this unnecessary request (it doesn't really hurt for now though).

@dlang-bot
Copy link
Collaborator

dlang-bot commented Jun 23, 2017

Thanks for your pull request, @wilzbach!

@wilzbach wilzbach force-pushed the fix-65 branch 2 times, most recently from b9d4b9b to 05fd31b Compare June 23, 2017 13:07
import std.algorithm : canFind, filter, map, sort, uniq;
import std.array : array;
// references are already sorted by id
auto bugzillaIds = refs.map!(r => r.id).uniq.array;
Copy link
Member

Choose a reason for hiding this comment

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

No need for .array here.

@wilzbach wilzbach force-pushed the fix-65 branch 2 times, most recently from c49bc6a to 26705de Compare June 24, 2017 20:47
.map!(i => i.severity)
.array
.sort()
.uniq;
Copy link
Member

Choose a reason for hiding this comment

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

(This comment is obsoleted if we agree on my other one below.)

There's no need for .array.sort().uniq:

  • You don't need uniq for canFind
  • Sorting is only used/needed for uniq, since plain linear search is faster without sorting than if you sort first ( O(n) vs O(n*log(n) + log(n)) )
  • uniq hides the SortedRange properties, so you *find won't perform binary search with UniqRange
  • array is not needed since sorting is not needed
    (Sorry for not pointing out this earlier, but I just looked at the code more thoroughly)

if (bugzillSeverities.canFind("enhancement"))
labels ~= "Enhancement";
else
labels ~= "Bug fix";
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, it seams that .array.sort.uniq.array may be needed after all.
That's because I think we need to add "Bug fix" label if there are more than one bugs attached to a PR and at least one of those bugs is not an enhancement request. Probably quite rare, and that's why I think it's worth labeling them.

if (bugzillSeverities.canFind("enhancement"))
    labels ~= "Enhancement";
if (bugzillSeverities.length > 2)
    labels ~= "Bug fix";

@wilzbach
Copy link
Contributor Author

(This comment is obsoleted if we agree on my other one below.)
There's no need for .array.sort().uniq:

Hmm, it seams that .array.sort.uniq.array may be needed after all.
That's because I think we need to add "Bug fix" label if there are more than one bugs attached to a PR and at least one of those bugs is not an enhancement request. Probably quite rare, and that's why I think it's worth labeling them.

Not really sure about this. This should be very rare. Let's KISS and drop .array.sort.uniq then.

@PetarKirov PetarKirov merged commit c547fb8 into dlang:master Jun 25, 2017
@wilzbach
Copy link
Contributor Author

Never say never: dlang/phobos#5511 (comment)

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