Extend Maps support.

[blackwinter]

Resolves #85.

[fsteeg]

I think draft status and request for review are contradicting?

[blackwinter]

I didn't realize that GitHub already sent out the request. It still offers a button "Ready for review". I assumed it would only actually request a review once the pull request is converted.

I'm waiting for the corresponding issue to become "ready".

[fsteeg]

I'm waiting for the corresponding issue to become "ready".

In our team process, the main point of readiness is making sure the person who will implement the issue knows enough to start. So here, as you already implemented it, it basically was ready :-) For our inter-team process for working on metafix, maybe we can work a bit differently here. Like, we could assume functional review is always done by @TobiasNx and code review is always done by me if you implement and vice versa. Then, whenever one of us feels something is ready, we can add the reviewers and move it to ready.

[blackwinter]

I'm still trying to adjust to that process. While at the same time trying to keep the pace up.

In this particular case (as well as #88 just now), I was also expecting to receive some kind of feedback whether the proposed changes are acceptable (e.g., a 👍 like you did on #88 😃). Or do you think that would be part of the code review? Are "placeholder" issues sufficient in those cases where the associated pull request is already waiting in line?

Like, we could assume functional review is always done by @TobiasNx and code review is always done by me if you implement and vice versa. Then, whenever one of us feels something is ready, we can add the reviewers and move it to ready.

👍 (if okay with @TobiasNx; for my own issues, I'm also - necessarily - doing a functional review anyway)

[fsteeg]

In this particular case (as well as #88 just now), I was also expecting to receive some kind of feedback whether the proposed changes are acceptable (e.g., a +1 like you did on #88 smiley).

Right, how about this: when we open a new issue, we add a description and proposed reviewers in the initial comment. When both reviewers add the 👍 reaction to that comment, the issue can be moved to ready.

blackwinter marked this pull request as ready for review 2 hours ago

The process here would be: assign #85 to @TobiasNx and move it to review, at the same time opening an unassigned PR (this one). Then, after the +1 by @TobiasNx in #85, he assigns the corresponding PR (this one) to the code reviewer specified in the issue. So we only start code review after the implemented functionality is reviewed as good, to avoid redundant code reviews of interim states. We don't need to stick to that process if it doesn't work for us here, but that's the idea.

[blackwinter]

Okay, I'll do my best to follow this process.

[blackwinter]

Some more thoughts...

In our team process, the main point of readiness is making sure the person who will implement the issue knows enough to start.

I'm not sure that's sufficient. There are four roles involved (not necessarily four people): reporter, implementer, functional reviewer, code reviewer. All of them have to come to a mutual understanding of what the issue entails. Otherwise they can't implement and/or review properly.

So we only start code review after the implemented functionality is reviewed as good, to avoid redundant code reviews of interim states.

That sounds nice in theory, but how relevant is it in practice? I'd wager a guess that adequate unit tests (which are mandatory and should be thoroughly examined during code review) would be strong indicators for functional soundness. They don't obviate the need for functional review, of course, but it doesn't have to be a prerequisite for code review either. OTOH, it might also be possible (if not essential) that a functional review has to be redone after changes from the code review.

But even if we should decide to do both reviews in parallel, I will have to remember to wait with the merge until functional review is done as well ;)

[fsteeg]

I'm not sure that's sufficient. There are four roles involved (not necessarily four people): reporter, implementer, functional reviewer, code reviewer. All of them have to come to a mutual understanding of what the issue entails. Otherwise they can't implement and/or review properly.

Right, and with the 👍 reaction process from above I think that works. What I didn't mention was that the issue has to be assigned to be ready. If the issue was not self-assigned, the assignee should probably 👍 the comment too. Then all 4 roles are in agreement, right?

I'd wager a guess that adequate unit tests (which are mandatory and should be thoroughly examined during code review) would be strong indicators for functional soundness. They don't obviate the need for functional review, of course, but it doesn't have to be a prerequisite for code review either. OTOH, it might also be possible (if not essential) that a functional review has to be redone after changes from the code review.

Yes those points are true. +1 for trying functional and code review in parallel. What do you think @TobiasNx?

[blackwinter]

Then all 4 roles are in agreement, right?

Yes.

The only issue with that is that reactions don't trigger notifications. But let's give it a try.

[fsteeg]

👍

[blackwinter]

Thanks :)