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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃彈 Prevent the use of mixed operators #16008

Merged
merged 1 commit into from Jun 13, 2018
Merged

馃彈 Prevent the use of mixed operators #16008

merged 1 commit into from Jun 13, 2018

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Jun 13, 2018

Warning mode on master, error mode for PR checks.

Fixes #16002

@rsimha rsimha self-assigned this Jun 13, 2018
@rsimha
Copy link
Contributor Author

rsimha commented Jun 13, 2018

/to @jridgewell @gmajoulet

Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

Thanks!!

@danielrozenberg
Copy link
Member

We can try doing an automated refactor to fix the existing warnings, but the tool I've been using (grasp) requires some more human oversight than I'm willing to give it at the moment.

After running gulp lint and finding which files have which mixed operators, I tried running (grasp)[http://www.graspjs.com/] using e.g., grasp --context 2 --squery 'logic[op="||"][right.op="&&"]' --replace '{{.left}} || ({{.right}})' --in-place [list of .js files], but different mixed rules have different squery search strings and different replacement rules, and grasp messes with formatting, so really it means going through hundreds of files manually, which we can do later

@rsimha
Copy link
Contributor Author

rsimha commented Jun 13, 2018

The idea of this PR is to gradually have these fixed as people edit files. Meanwhile, I agree that if you are able to do a mass refactor and reliably fix all outstanding issues, it's worth doing.

@rsimha rsimha merged commit 34b4e16 into ampproject:master Jun 13, 2018
@rsimha rsimha deleted the 2018-06-12-MixedOperators branch June 13, 2018 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants