Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add show-backlog command #168

Merged
merged 1 commit into from
Mar 4, 2018
Merged

Add show-backlog command #168

merged 1 commit into from
Mar 4, 2018

Conversation

matheussbernardo
Copy link
Contributor

This PR starts to work on #45,

I've created a method on ScrumBoard to get the Sprint Backlog Column and then on the CLI I created the command and printed the information.

The --velocity option on the #45 is WIP.

Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

@matheussbernardo thanks a lot for your PR! 😍

As commented in #45 (comment), forget about the velocity and just focus on the command. 😄

I have made some comments, if you have doubts or comments, please do not hesitate to ask! 😄

lib/cli.rb Outdated
option 'board-id', desc: 'Id of Trello board', required: true
def show_backlog
puts 'Title'
TrelloWrapper.new(@@settings).board(options['board-id']).sprint_backlog_column.cards.each do |card|
Copy link
Member

Choose a reason for hiding this comment

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

if I am not wrong, this should be planning_backlog_column and not sprint_backlog_column.

@cornelius wrote in #45 (comment):

When doing sprint commitments we need a way to see the whole backlog in one view.

Normally when preparing the next Sprint in a team using Scrum, you have to decide how many task the team is able to do in the Sprint. That why I think this is in the planning board and not in the sprint one. @cornelius is that right?

Note: A sprint can be for example two week. The team decide at the start what they are going to work in this two week. @matheussbernardo if you think it would be useful to read more about how scrum works, please tell me and I will send you some material. 😉

Copy link
Member

Choose a reason for hiding this comment

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

Also, what about writing this like:

trello = TrelloWrapper.new(@@settings)
board = trello.board(board_id(options['board-id']))


board.planning_backlog_column.cards.each do |card|

More readable when splitting it 😉

lib/cli.rb Outdated
desc 'show-backlog', 'Show backlog of board'
option 'board-id', desc: 'Id of Trello board', required: true
def show_backlog
puts 'Title'
Copy link
Member

Choose a reason for hiding this comment

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

puts '| Title' 😉

@@ -12,6 +12,10 @@ def columns
@columns ||= @board_data['lists'].map{|x| Column.new(@board_data, x['id'], @settings)}
end

def sprint_backlog_column
columns.select{ |col| col.name == 'Sprint Backlog' }.first
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to write column instead of col, more explicit 😉

The column name can be configured, as you can see in the Documentation, so you should use settings.list_names['planning_backlog'] 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @Ana06, the method list_names does not exist in the setting object, what I did was @settings.scrum['list_names']['planning_backlog']

@cornelius
Copy link
Member

Yes, this is about the backlog on the planning board.

@matheussbernardo
Copy link
Contributor Author

@Ana06 I updated as requested.

Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

LGTM, but we need a test! 😉 Can you add one? 🙏

And please rebase 😄

@Ana06
Copy link
Member

Ana06 commented Mar 1, 2018

@matheussbernardo do you need help? The code is already great, we just need a test and we can 🚢 it! 😉

@matheussbernardo
Copy link
Contributor Author

@Ana06 Sorry for the delay, I was on vacation! I added the requested spec. Is there anything else to do?

Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

@matheussbernardo

@Ana06 Sorry for the delay, I was on vacation! I added the requested spec. Is there anything else to do?

It is fine, I just wanted to know if you needed help 😉 The code looks great! 😍 Can you please squash your commits? that there is only one commit (two if you want to have the test separated).
The commit should include a title (Add show-backlog command) a description and the issue it is closing. 😉

@matheussbernardo
Copy link
Contributor Author

@Ana06 Is the squash correct? I`m having a hard time with git rebase -i.

@Ana06
Copy link
Member

Ana06 commented Mar 3, 2018

@matheussbernardo

@Ana06 Is the squash correct? I`m having a hard time with git rebase -i.

It doesn't seem so 😭 rebase -i is always tricky the first times 😉 So, first of all make a copy pf your changes, in case you destroy everything by accident.

Try with:

git remote add upstream https://github.com/openSUSE/trollolo.git
git fetch upstream
git rebase upstream/master

And then push it again. This should remove the current commits that are in your PR and that are not yours.

In case that this doesn't work and you don't manage to solve it, you can also ping me in IRC on Freenode (normally my nick is @Ana06, although sometimes it has a random number of 0s). Maybe easier 😉.

@matheussbernardo
Copy link
Contributor Author

@Ana06 It worked! Hope we can 🚢 it now!

Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

@matheussbernardo

@Ana06 It worked! Hope we can :ship: it now!

🎉

Normally the description should go in a different line, so it should look like:

Add show-backlog command

It creates a command that shows the planning backlog.

It closes issue #45.

But it is not that important, let's merge it. For future commits, you can read: https://github.com/RomuloOliveira/commit-messages-guide

Great work! 💐

@Ana06 Ana06 merged commit 484b921 into openSUSE:master Mar 4, 2018
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