LP2Kanban

Code review comment for lp:~bjornt/lp2kanban/bugs-to-cards

bugs-to-cards
Merge into trunk

Revision history for this message

Björn Tillenius (bjornt) wrote on 2012-11-29:

On Wed, Nov 28, 2012 at 02:39:57PM -0000, Benji York wrote:
> Review: Approve code
>
> The branch looks good. I didn't see anything that should prevent this
> from landing but I had several comments/suggestions while reading the
> code.
>
> > I'm not completely happy with 3), since it adds noise to the bug and
> > requires authenticated use of the API, but it's needed, since
> > otherwise the list of bugs to process would just grow and grow.
>
> An idea how to tag cards as synced with kanban without having to remove
> the tags later: Tag bugs that should be synced, the script then queries
> for non-closed, tagged bugs and compares against existing cards,
> creating those that do not yet exist.
>
> We will be processing a few more bugs than is strictly necessary, but
> once a bug is closed it will fall out of the set of bugs being
> inspected, so the number of bugs examined will be bounded.
>
> This approach also means that "semantic" tags can be used to manage
> cards. For example, if you have a project to implement the "gizmo"
> feature and all the "gizmo" bugs are already tagged, you can just start
> running lp2kanban with the new tag name and all the bugs will have cards
> created and the tags won't be removed (as they would now).
>
> What do you think about that?

Hmm. Maybe. I'm going to try it and do some timings. I currently run the
sync script to create new bugs once every minute, and would like to keep
doing so.

It also raises the question, how to delete cards from the board.
Although, I guess I could make it a config option whether to remove the
tag...

> Config option brainstorming
> ---------------------------
>
> bug_new_card_tag:
>
> - card_creation_bug_tag
> - new_card_bug_tag
> - bug_to_card_tag
>
> bug_new_card_type:
>
> - bug_to_card_type
>
>
> I like the last two suggestions in each category because they share a
> prefix and "bug_to_card" is relatively obvious in meaning.

Yeah, those are good suggestion. Let's see, I will postpone merging this
branch a while, to see whether I have time to implement some other
features I want. One would be to have a default card type for new bugs,
but then being able to map specific tags to specific types. For example,
the default could be 'Feature', but if a bug is marked with a 'defect'
tag, the card type will be 'Bug'.

> Is my reading correct in that create_new_cards creates all the bugs in
> the left-most lane? It would be nice for that to be configurable, many
> boards aren't structured that way. In fact, I can see people running
> multiple passes; creating cards from one tag in one lane and cards from
> another tag in another.

Ah, that is correct, that's something I forgot to address before
proposing the merge. I'll ponder on how to do that in a nice way.

> Hmm, maybe that doesn't matter because the cards are then synced after
> they are created, thus moving them into the correct lanes. If that is
> correct, then a comment to that effect in create_new_cards would be
> helpful.

No, I don't think that will work. As far I as can see, the logic for
moving lanes are highly specific to a certain board layout, and it will
move cards only within the same horizontal lane. I'm currently trying to
extend that logic to work with our board layout as well.

Could you please show me how your board look like? It would be useful,
to make sure I don't break anything.

> Shouldn't the newly created cards have the "Synced with Launchpad"
> marker added to the description? Wait, since the cards are "dirty",
> that will be done by save_board, right?

Maybe... I haven't cared about that so far, since we couldn't use the
sync logic. I'll investigate, though...

>
>
> The tests are appreciated.
>
> We could use some tests for create_new_cards too. I would include tests
> for the lane lookup code (possibly breaking it out into its own
> function) and for the card type name to card type ID translation
> function (it would probably benefit from being its own function too).

Yeah. create_new_cards is untested, probably for the same reason why
most of lp2kanban is untested; it requires quite complicated mock
objects for the LP and kanban APIs.

I'll see if I can split it up a bit more to improve the testing.

--
Björn Tillenius | https://launchpad.net/~bjornt

On Wed, Nov 28, 2012 at 02:39:57PM -0000, Benji York wrote:
> Review: Approve code
> 
> The branch looks good.  I didn't see anything that should prevent this
> from landing but I had several comments/suggestions while reading the
> code.
> 
> > I'm not completely happy with 3), since it adds noise to the bug and
> > requires authenticated use of the API, but it's needed, since
> > otherwise the list of bugs to process would just grow and grow.
> 
> An idea how to tag cards as synced with kanban without having to remove
> the tags later: Tag bugs that should be synced, the script then queries
> for non-closed, tagged bugs and compares against existing cards,
> creating those that do not yet exist.
> 
> We will be processing a few more bugs than is strictly necessary, but
> once a bug is closed it will fall out of the set of bugs being
> inspected, so the number of bugs examined will be bounded.
>
> This approach also means that "semantic" tags can be used to manage
> cards.  For example, if you have a project to implement the "gizmo"
> feature and all the "gizmo" bugs are already tagged, you can just start
> running lp2kanban with the new tag name and all the bugs will have cards
> created and the tags won't be removed (as they would now).
> 
> What do you think about that?

Hmm. Maybe. I'm going to try it and do some timings. I currently run the
sync script to create new bugs once every minute, and would like to keep
doing so.

It also raises the question, how to delete cards from the board.
Although, I guess I could make it a config option whether to remove the
tag...

> Config option brainstorming
> ---------------------------
> 
> bug_new_card_tag:
> 
> - card_creation_bug_tag
> - new_card_bug_tag
> - bug_to_card_tag
> 
> bug_new_card_type:
> 
> - bug_to_card_type
> 
> 
> I like the last two suggestions in each category because they share a
> prefix and "bug_to_card" is relatively obvious in meaning.

> Is my reading correct in that create_new_cards creates all the bugs in
> the left-most lane?  It would be nice for that to be configurable, many
> boards aren't structured that way.  In fact, I can see people running
> multiple passes; creating cards from one tag in one lane and cards from
> another tag in another.

Ah, that is correct, that's something I forgot to address before
proposing the merge. I'll ponder on how to do that in a nice way.

> Hmm, maybe that doesn't matter because the cards are then synced after
> they are created, thus moving them into the correct lanes.  If that is
> correct, then a comment to that effect in create_new_cards would be
> helpful.

Could you please show me how your board look like? It would be useful,
to make sure I don't break anything.

> Shouldn't the newly created cards have the "Synced with Launchpad"
> marker added to the description?  Wait, since the cards are "dirty",
> that will be done by save_board, right?

Maybe... I haven't cared about that so far, since we couldn't use the
sync logic. I'll investigate, though...

> 
> 
> The tests are appreciated.  
> 
> We could use some tests for create_new_cards too.  I would include tests
> for the lane lookup code (possibly breaking it out into its own
> function) and for the card type name to card type ID translation
> function (it would probably benefit from being its own function too).

Yeah. create_new_cards is untested, probably for the same reason why
most of lp2kanban is untested; it requires quite complicated mock
objects for the LP and kanban APIs.

I'll see if I can split it up a bit more to improve the testing.

-- 
Björn Tillenius | https://launchpad.net/~bjornt

« Back to merge proposal