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

drop null from SharedFlagsSettings properties #7788

Closed
brendankenny opened this issue Mar 29, 2019 · 3 comments
Closed

drop null from SharedFlagsSettings properties #7788

brendankenny opened this issue Mar 29, 2019 · 3 comments

Comments

@brendankenny
Copy link
Member

brendankenny commented Mar 29, 2019

Several SharedFlagsSettings are arrays but are both optional and nullable for historical reasons. We should drop null as a possible value.

I believe they should all be compatible with this change (an empty array would become the replacement null state, and we'd have to check .length instead of just falsiness), though this will require some fixes in config.js and gather-runner.js.

Original motivation was to clean up for representation in proto form, but it's also just excessive complexity when trying to understand allowed flag values.

Affected properties are blockedUrlPatterns, additionalTraceCategories, onlyAudits, onlyCategories, skipAudits, extraHeaders, and precomputedLanternData.

@patrickhulce
Copy link
Collaborator

I think it's teensy bit problematic in some of the cases to use [] for them (like only audits, I can see it being misunderstood to mean run no audits, but then again what on earth would you expect to happen in that case, so seems fine 👍 )

What is your thinking on the replacement null value for precomputedLanternData? that's the only one I see here where [] or {} doesn't really work

@brendankenny
Copy link
Member Author

What is your thinking on the replacement null value for precomputedLanternData?

we can ask @exterkamp, but I may have spoken too soon on that one and extraHeaders. I think the main concern was repeated fields, and they aren't an array.

like only audits, I can see it being misunderstood to mean run no audits, but then again what on earth would you expect to happen in that case

yeah, I see both sides too. I feel like there is a way to express what we mean here that also keeps proto lists and default values happy.

With less of a need for proto config settings, though, maybe this is a v6 breaking change. We probably have enough decisions still to make on our plate

@paulirish paulirish mentioned this issue Apr 16, 2019
67 tasks
@brendankenny
Copy link
Member Author

we could clean some of these up, but nothing is falling over from these excess nulls, and the proto move we thought we were going to make for settings isn't going to happen, so I'm going to close this issue. We can revisit if someday something forces us to care :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants