Skip to content

Conversation

@josenavas
Copy link
Contributor

Built on top of #1203 (review/merge that one first)

This PR modifies the RawData object to reflect the changes in the DB. It also fixes the tests for the RawData object.

All the other tests are still expected to fail.

@josenavas josenavas added this to the Alpha 0.2 milestone May 24, 2015
@josenavas josenavas mentioned this pull request May 24, 2015
qiita_db/data.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Just checking, is it really an IncompetentQiitaDeveloperError or a QiitaError? I'm thinking than a user can hit this error from IPython.

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 call, done

@antgonza
Copy link
Member

1 minor comment so 👍

qiita_db/data.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't -> don't

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this method not used anywhere else? That's surprising 👀

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, it was not used, sorry I forgot to comment that on the PR. I checked and it was not used. This function was intended to remove a single filepath from the raw data, but we are not supporting right now because it gets tricky: we are checking if the raw data has the same number of barcode files and forward reads, so removing a single file will break this checks. Thus, we are allowing just removing all using clear_filepaths

Also, this function was using purge_filepaths instead of move_filepaths_to_upload_folder.

Given that it was not used, and I don't see a use for it soon; and it's a developer burden to maintain it, I think is better to remove it and we will re-add it if we actually need it.

@ElDeveloper
Copy link
Contributor

Very minor comments, should be good to go after.

@josenavas
Copy link
Contributor Author

Comments should be addressed @antgonza @ElDeveloper

@ElDeveloper
Copy link
Contributor

Got it, thanks! 👍 ready for merge.

On (May-27-15|16:03), josenavas wrote:

Comments should be addressed @antgonza @ElDeveloper


Reply to this email directly or view it on GitHub:
#1204 (comment)

antgonza added a commit that referenced this pull request May 27, 2015
@antgonza antgonza merged commit 7b227a3 into qiita-spots:fix-1084 May 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants