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

Small fixes #329

Merged
merged 3 commits into from
Nov 9, 2018
Merged

Small fixes #329

merged 3 commits into from
Nov 9, 2018

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

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.

None yet

2 participants