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

Fix crash due to engine list not being filtered upon partial results #9

Merged
merged 3 commits into from
Mar 13, 2017

Conversation

vovimayhem
Copy link
Member

@vovimayhem vovimayhem commented Mar 13, 2017

What does this PR do?

Addresses Issue #8:

  • Updated project layout
  • Implements the engine filtering with partial results

Closes #8
Closes #9

@vovimayhem vovimayhem changed the title [WIP] Fix crash due to engine list not being filtered upon linguist results [WIP] Fix crash due to engine list not being filtered upon partial results Mar 13, 2017
@vovimayhem vovimayhem force-pushed the fix/crash-due-to-not-filtering-engines branch from baf22c2 to c7a9a78 Compare March 13, 2017 20:11
@vovimayhem vovimayhem changed the title [WIP] Fix crash due to engine list not being filtered upon partial results Fix crash due to engine list not being filtered upon partial results Mar 13, 2017
@vovimayhem
Copy link
Member Author

@thelastinuit Nachito, it's ready. Review it


dependency_features = @run_rules.fetch('features', []).sort
features_found = partially_detected_features.map(&:name).flatten.uniq
dependency_features_are_met = (dependency_features & features_found).sort == dependency_features
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on moving former blocks into another method? Just for trying to remove some duplicity.. Something like:

def can_run? (partially_detected_features)
   return true unless @run_rules.present? 
   dependencies_are_met?('engines', partially_detected_features) && dependencies_are_met?('features', partially_detected_features) 
end


def dependencies_engines_are_met? (rule_type, partially_detected_features)
    dependencies = dependencies_for(rule_type)
    features = if rule_type == "engines" 
                   partially_detected_features.map(&:engines).flatten.uniq
                else
                   partially_detected_features.map(&:name).flatten.uniq
                end
    (dependencies & features).sort == dependencies 
end

def dependencies_for rule_type
   @run_rules.fetch(rule_type, []).sort
end

Copy link
Member

Choose a reason for hiding this comment

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

Of we can use your original structure:

def can_run? (partially detected_features)
    return true unless @run_rules.present? 
   dependency_engines_are_met?(partially_detected_features) && dependency_features_are_met?(partially_detected_features)
end

def dependency_engines_are_met partially detected_features
     dependency_engines = rules_for('engines')
     engines_ran = partially_detected_features.map(&:engines).flatten.uniq
     dependency_engines_are_met = (dependency_engines & engines_ran).sort == dependency_engines
end

def dependency_features_are_met partially_detected_features
    dependency_features = rules_for('features')
    features_found = partially_detected_features.map(&:name).flatten.uniq
    dependency_features_are_met = (dependency_features & features_found).sort == dependency_features
end

def rules_for type
   @run_rules.fetch(type, []).sort
end

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point... Let me rinse & repeat this guy

Copy link
Member

@mayra-cabrera mayra-cabrera left a comment

Choose a reason for hiding this comment

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

@vovimayhem LGTM 🎉

@vovimayhem
Copy link
Member Author

LGTM TOO :D

@vovimayhem vovimayhem merged commit 179cc7f into master Mar 13, 2017
@mayra-cabrera mayra-cabrera deleted the fix/crash-due-to-not-filtering-engines branch March 19, 2017 22:28
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.

None yet

3 participants