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.

Sign up for GitHub

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

Jump to bottom

add dummy vars for define flags warning suppression #3874

Closed
wants to merge 1 commit into from
Closed

add dummy vars for define flags warning suppression #3874

wants to merge 1 commit into from

Conversation

erwinmombay
Copy link
Member

@erwinmombay erwinmombay commented Jul 1, 2016 •
edited
Loading

supress the warnings we get during compilation for the custom flags we pass to our AmpCommandLineRunner class

@erwinmombay
Copy link
Member Author

@cramforce kind of ugly, could also do a Builder but didn't seem necessary with just 2 flags.

@erwinmombay erwinmombay changed the title remove custom define arguments remove custom define arguments from array passed to base CommandLineRunner Jul 1, 2016
@erwinmombay
Copy link
Member Author

will wait till #3887 is merged so i can recompile with my changes and dont just over write the jar

@cramforce

@cramforce
Copy link
Member

Merged #3887

@erwinmombay
Copy link
Member Author

fixed conflicts and recompiled runner.jar

@erwinmombay erwinmombay force-pushed the remove-closure-warnings branch 2 times, most recently from 08d3c7a to 4762ea8 Compare July 6, 2016 02:25
@erwinmombay erwinmombay changed the title remove custom define arguments from array passed to base CommandLineRunner add dummy vars for define flags warning suppression Jul 6, 2016
@erwinmombay
Copy link
Member Author

@cramforce so i tried using dummy vars, and did a full reduction (test case was 2 simple js files) and --define stops doing the replace when theres an import

@erwinmombay
Copy link
Member Author

will revert this back to the old code with the java argument filter

@erwinmombay
Copy link
Member Author

ping @cramforce do you know why --define doesn't work here? i did a full reduction of 2 simple files and the output wouldn't have the value replaced one i got into 2 modules

cramforce
cramforce reviewed Jul 11, 2016
View reviewed changes
src/amp.js
@@ -36,6 +36,15 @@ import {cssText} from '../build/css';
import {maybeValidate} from './validator-integration';
import {maybeTrackImpression} from './impression';

/** @define @const {boolean} */
Copy link
Member

Choose a reason for hiding this comment

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

Definitely leave out the @const

Otherwise, no idea.

@erwinmombay erwinmombay deleted the remove-closure-warnings branch July 22, 2016 19:55
@erwinmombay
Copy link
Member Author

fixed by @aghassemi unknownDefine=off work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cramforce cramforce

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@erwinmombay @cramforce