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

options and terminology fixes and docs added to extension code #614

Merged
merged 1 commit into from
Aug 28, 2016

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Aug 25, 2016

cleanup of extension code, which has grown a little hairy as it has accumulated. Mostly:

  • always start with a freshly parsed config.json when running lighthouse
  • finish removal of audit whitelist in filterPasses -> validatePasses #608 by filtering audits in config directly before passing into LH, but make selected audit categories work again.
  • s/audits/aggregations after Save audit list into storage so it's kept for the next run #595, to reflect that it's aggregation categories being saved and loaded for the options list, not audits themselves
  • added some docs so it's hopefully a little faster to find your way around in there

This PR fixes 2 bugs:

  1. the audit whitelist was still being passed into the Config constructor. Amazingly, the browserify polyfill for path.dirname parses new Set(['array', 'of', 'audit', 'names']) as '.', so this wasn't actually causing any problems.
  2. Currently on master the options page has no affect on the audits run. This makes that work again

@brendankenny brendankenny changed the title terminology fixes and docs added to extension terminology fixes and docs added to extension code Aug 25, 2016
@brendankenny brendankenny changed the title terminology fixes and docs added to extension code options and terminology fixes and docs added to extension code Aug 26, 2016
const log = require('../../../lighthouse-core/lib/log');

const defaultConfigJSON = JSON.stringify(defaultConfig);
Copy link
Member

Choose a reason for hiding this comment

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

lets do this stringify down where we parse. too many configs running about :)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@paulirish paulirish added the lgtm label Aug 27, 2016
@brendankenny brendankenny merged commit 39ae86a into master Aug 28, 2016
@brendankenny brendankenny deleted the extensionclean branch August 28, 2016 22:23
background.loadSelectedAggregations()
.then(getAuditsFromSelected)
.then(selectedAudits => {
return background.runLighthouse({
Copy link
Collaborator

Choose a reason for hiding this comment

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

@brendankenny why use a return here?

Copy link
Member Author

Choose a reason for hiding this comment

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

just a (debatable :) readability improvement. It's often easy to miss that a multiline body on an arrow function is being returned

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.

3 participants