|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
58 bytes, text/x-review-board-request
|Details | Review|
Created attachment 8741226 [details] MozReview Request: Bug 1264527 - Remove wanted_mozconfig_variables. r?nalexander While forgetting about it was warned about, having to add every new environment option to wanted_mozconfig_variables is cumbersome. It turns out there is a hackish way to make things work without that list, which, all things considered, is not worse than the hacks around the wanted_mozconfig_variables function, and are certainly an improvement as it doesn't require an ever growing list of environment options. Review commit: https://reviewboard.mozilla.org/r/46311/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46311/
Attachment #8741226 - Flags: review?(nalexander)
Comment on attachment 8741226 [details] MozReview Request: Bug 1264527 - Remove wanted_mozconfig_variables. r?nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46311/diff/1-2/
Comment on attachment 8741226 [details] MozReview Request: Bug 1264527 - Remove wanted_mozconfig_variables. r?nalexander https://reviewboard.mozilla.org/r/46311/#review43049 Wow, that is hacky, but I'm fine with this approach while we transition. ::: build/moz.configure/init.configure:272 (Diff revision 2) > # contraints that don't really apply to the command-line > # emulation that mozconfig provides. > helper.add(arg, origin='mozconfig', args=helper._args) > > def add(key, value): > - # See comment above wanted_mozconfig_variables > + if key.isupper(): I generally prefer early return rather than indentation.
Attachment #8741226 - Flags: review?(nalexander) → review+
https://reviewboard.mozilla.org/r/46311/#review43049 > I generally prefer early return rather than indentation. For short functions with little level of nesting, I generally prefer not doing early returns. That said, early returns would make the remaining_mozconfig_options function a little better. So I'll do that there.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.