Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

Add support for config payload for AuditConfiguration #165

Merged
merged 3 commits into from
Jul 25, 2015

Conversation

loginx
Copy link
Collaborator

@loginx loginx commented Jun 10, 2015

* Overwrite `AuditConfiguration` properties defined in the constructor
with properties found in payload parameter.
* Add test for new behaviour.
* Add usage example in docs.
* Fixes GoogleChrome#164
*/
axs.AuditConfiguration = function() {
axs.AuditConfiguration = function(config) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool! I like being able to pass a config object. Me gusta.

@ckundo
Copy link
Collaborator

ckundo commented Jun 12, 2015

This is an exciting change, great work! LGTM

@@ -78,6 +83,12 @@ axs.AuditConfiguration = function() {
*/
this.showUnsupportedRulesWarning = true;

for (var prop in this) {
if ((this.hasOwnProperty(prop)) && (prop in config)) {
this[prop] = config[prop];
Copy link
Contributor

Choose a reason for hiding this comment

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

This is cool, but is not going to work for severity, ignoreSelectors or ruleConfig. We'll probably need to special case those.

@loginx
Copy link
Collaborator Author

loginx commented Jun 23, 2015

@alice That's correct. That can be done but we have to determine a reasonable format for the data to pass to these methods in the config payload.

It would probably be a good idea to merge this in first, and open an issue for automating these methods through config as well.

@@ -22,8 +22,13 @@ goog.provide('axs.AuditConfiguration');
/**
* Object to hold configuration for an Audit run.
* @constructor
* @param {?Object=} config Configuration object
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add more detailed documentation for this, stating what can and cannot be configured this way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do.

@ckundo
Copy link
Collaborator

ckundo commented Jul 24, 2015

Looking forward to getting this merged! @loginx do you need an support on this?

@loginx
Copy link
Collaborator Author

loginx commented Jul 24, 2015

@ckundo I completely forgot about this. I thought it had been merged long ago. I'll have a look at this tonight as well.

@alice
Copy link
Contributor

alice commented Jul 25, 2015

👍 go ahead and pull

loginx added a commit that referenced this pull request Jul 25, 2015
Add support for config payload for AuditConfiguration
@loginx loginx merged commit a0fe269 into GoogleChrome:master Jul 25, 2015
@loginx loginx deleted the auditconfiguration-config-payload branch July 25, 2015 00:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants