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

More fixes on the Qiita(Status)Object classes #73

Merged
merged 4 commits into from
Jun 9, 2014

Conversation

josenavas
Copy link
Contributor

Some fixes on the QiitaObject and QiitaStatusObject base classes:

  • The __init__ on the QiitaObject class now checks that the id passed actually belongs to an object in the database.
  • The status getter/setter can be written in the base class. Just need to define a new function _status_setter_checks that should be overwritten by the subclasses in order to perform the necessary checks before updating the DB object.
  • The check_status decorator was not working. Rewriting it to be just a boolean function, so subclasses can call it when needed.
  • The documentation in the entire file was not following the numpy standard. I've updated the documentation.

It would be great if this can be in soon, as it should be merged before any other object is merged.

@coveralls
Copy link

Coverage Status

Coverage increased (+4.04%) when pulling 0ae0276 on josenavas:init_fix into 162cb0f on biocore:master.

"""
if not self._check_id(id_):
raise QiitaDBObjectDoesNotExistsError(id_, self._table)
Copy link
Contributor

Choose a reason for hiding this comment

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

That is a hugely long error name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I had the same feeling. Do we actually need to prefix the error with QiitaDB? Then we can have:
ObjectDoesNotExistsError, which is a bit better...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking a bit more, QiitaDBUnknownIDError looks better...

@squirrelo
Copy link
Contributor

A few comments, overall looks good.

@josenavas
Copy link
Contributor Author

Thanks @squirrelo I've solved your comments.
Any other pair of 👀 that want to review this?

@coveralls
Copy link

Coverage Status

Coverage increased (+4.04%) when pulling bac600d on josenavas:init_fix into 162cb0f on biocore:master.

@wasade
Copy link
Contributor

wasade commented Jun 8, 2014

Seems good

@josenavas
Copy link
Contributor Author

looks like we have 2 👍 can somebody take a quick look and merge?

antgonza added a commit that referenced this pull request Jun 9, 2014
More fixes on the Qiita(Status)Object classes
@antgonza antgonza merged commit a120cce into qiita-spots:master Jun 9, 2014
@josenavas josenavas deleted the init_fix branch June 10, 2014 01:43
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.

5 participants