Skip to content

Conversation

penguineer
Copy link
Contributor

@penguineer penguineer commented Oct 19, 2018

Small fixes to make life easier:

  • Todo model checks types in __setattr__
  • Formatter distinguishes text and lists in _columnize
  • Check and handle empty values in _columize_*

(Work towards solving #323)

Copy link
Member

@WhyNotHugo WhyNotHugo left a comment

Choose a reason for hiding this comment

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

Sorry for the slow review; I kinda missed the notification email! 😓

LGTM, I'd just apply some minor changes to this.

Thanks!


return self._columnize_list(label, lines)

def _columnize_list(self, label, lst):
Copy link
Member

Choose a reason for hiding this comment

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

I'd move this function's logic inside _columnize_text, since it's not really something we're reusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I externalized this part on purpose, because we will use it for the categories. (see PR #323)

todoman/model.py Outdated
if value is None:
v = ''
elif not isinstance(value, str):
raise ValueError("Got {0} for {1} where str was expected"
Copy link
Member

Choose a reason for hiding this comment

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

These validations are kinda unreachable, and are mostly here to prevent programming errors. Because of that, I'd much prefer to use assert, since otherwise these is normally-unreachable code.

Copy link
Contributor Author

@penguineer penguineer Oct 25, 2018

Choose a reason for hiding this comment

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

Good point, I'll change this part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, changed it into assertions.

@penguineer penguineer mentioned this pull request Oct 25, 2018
@penguineer
Copy link
Contributor Author

penguineer commented Oct 25, 2018

Just realized that the (now gone) fourth commit fixes something that is already taken care of in #332.

I guess we should wait until this is merged and I'll rebase to the new master.

Copy link
Member

@WhyNotHugo WhyNotHugo left a comment

Choose a reason for hiding this comment

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

LGTM. I'll merge this once master is fixed, which I'll try to do very soon!

@WhyNotHugo
Copy link
Member

Can you merge or rebase master into your branch, so that CI passes?

Thanks!

@penguineer
Copy link
Contributor Author

Can you merge or rebase master into your branch, so that CI passes?

Thanks!

Done.

@WhyNotHugo WhyNotHugo merged commit f2a006b into pimutils:master Nov 9, 2018
@WhyNotHugo
Copy link
Member

Thanks! Sorry to keep you waiting so long with a broken master.

@penguineer
Copy link
Contributor Author

Thanks! Sorry to keep you waiting so long with a broken master.

No worries, thanks for merging!
We'll carry on with the original contribution now. :)

@penguineer penguineer deleted the fixes branch November 9, 2018 15:59
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