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

Support for comments #2

Merged
merged 4 commits into from
Oct 22, 2016
Merged

Support for comments #2

merged 4 commits into from
Oct 22, 2016

Conversation

schuellerf
Copy link
Contributor

@schuellerf schuellerf commented Oct 17, 2016

quick and dirty implementation to show comments

Copy link
Owner

@phipsgabler phipsgabler left a comment

Choose a reason for hiding this comment

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

That was on my todo list already, thanks.

As far as I understand, print_card now iterates over all actions every time, so we have O(#cards * #actions) checks. Could you implement it in such a way that all comments are at once extracted into a dictionary of lists or something like that, and this then gets passed to print_card? If the comments flag is False, you could just give it an empty dictionary.

@schuellerf
Copy link
Contributor Author

although I don't have a board where this might have a real impact on runtime
sure I'll change that...

@phipsgabler
Copy link
Owner

I actually expected that argument ;) It just felt wrong to me. Thanks for changing.

Now that I have found time, I reformatted the output a bit, so that it becomes a markdown list with prepended information. What do you think?

@schuellerf
Copy link
Contributor Author

Looks nice! The next thing would be i18n :)
Before my edits only content of trello itself was printed ...

@phipsgabler phipsgabler merged commit 9900b7a into phipsgabler:master Oct 22, 2016
@phipsgabler
Copy link
Owner

I have already thought about adding something like config files, but not really worked it out. There's now an issue #3 where we can discuss this.

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.

2 participants