Skip to content

Conversation

@josenavas
Copy link
Contributor

This adds the check_restrictions functionality to the metadata template objects. This functionality is needed to check the restrictions from the GUI and disable functionality as needed.

Should be easy to review!

@antgonza
Copy link
Member

antgonza commented May 2, 2015

Your friend pep(e)8 is calling ...

Copy link
Member

Choose a reason for hiding this comment

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

Will it be better to do this in a set operation?

@josenavas
Copy link
Contributor Author

Thanks @antgonza
Comments solved!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 79.33% when pulling 55944f2 on josenavas:add-check-restriction into 01ac199 on biocore:master.

@antgonza
Copy link
Member

antgonza commented May 2, 2015

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just do {col for restriction in restrictions for col in restriction.columns}? The generator is immediately consumed so it isn't clear what the benefit of lazy eval is here

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 was just avoiding the double list comprehension, as sometimes it has been criticized. I think it is not too complicated, so I'll change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense. in that case though, it would be better to just create the set
using two for loops. eg

cols = set()
for restriction in restrictions:
for col in restriction.columns:
cols.add(col)

On Mon, May 4, 2015 at 2:29 PM, josenavas notifications@github.com wrote:

In qiita_db/metadata_template/base_metadata_template.py
#1141 (comment):

  •    Parameters
    

  •    restrictions : list of Restriction
    
  •        The restrictions to test if the template fulfills
    
  •    Returns
    

  •    set of str
    
  •        The missing columns
    
  •    """
    
  •    def _col_iter():
    
  •        for restriction in restrictions:
    
  •            for col in restriction.columns:
    
  •                yield col
    

I was just avoiding the double list comprehension, as sometimes it has
been criticized. I think it is not too complicated, so I'll change it.


Reply to this email directly or view it on GitHub
https://github.com/biocore/qiita/pull/1141/files#r29620275.

@wasade
Copy link
Contributor

wasade commented May 4, 2015

Just one comment, 👍 otherwise

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 79.32% when pulling 20eebdb on josenavas:add-check-restriction into 01ac199 on biocore:master.

@josenavas
Copy link
Contributor Author

@antgonza @wasade any remaining comment in here?

wasade added a commit that referenced this pull request May 5, 2015
Adding check_restrictions functionality
@wasade wasade merged commit 5ff115e into qiita-spots:master May 5, 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.

4 participants