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

Ignore version file in eslint to resolve build error #559

Merged
merged 1 commit into from
Nov 15, 2018

Conversation

brodycj
Copy link
Contributor

@brodycj brodycj commented Nov 15, 2018

This change should to give us a green build again.

Rationale is that the eslint update introduced in 393dad6 checks most of the files under bin and will not accept JavaScript that uses double-quotes for strings. The workaround in 393dad6 was to change the generated bin/templates/cordova/version file to use single-quotes in the version string.

But since cordova-coho will regenerate bin/templates/cordova/version with double-quotes when updating the version number, we need a different solution.

This is a quick workaround solution. I won't have a problem if anyone wants to propose an improved solution. Alternative may be to update cordova-coho to generate version file with single-quotes.

Quick workaround, needed since cordova-coho
generates version file with double-quotes
@dpogue
Copy link
Member

dpogue commented Nov 15, 2018

There might be an eslint ignore comment we can wrap around the version line to suppress the error?

@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

There might be an eslint ignore comment we can wrap around the version line to suppress the error?

That would involve updating the cordova-coho tool. I would rather not spend time on updating cordova-coho right now.

@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

Passes on Travis CI now, will watch for the result on AppVeyor. Hope we can get this finished soon.

@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

Build is green, merging now

@brodycj brodycj merged commit f1396c7 into apache:master Nov 15, 2018
@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

That would involve updating the cordova-coho tool.

I may be wrong here. Not sure if I would favor an extra eslint comment though. Build of master branch is green again, moving on.

@brodycj brodycj deleted the eslint-ignore-version branch November 15, 2018 02:01
@dpogue
Copy link
Member

dpogue commented Nov 15, 2018

I may be wrong here. Not sure if I would favor an extra eslint comment though. Build of master branch is green again, moving on.

If the file is entirely generated by coho, then the best option is probably looking at how to generate it with the right quotes. The eslint comment suggestion was assuming that coho was just doing a find-and-replace on it in an existing file.

@janpio
Copy link
Member

janpio commented Nov 15, 2018

Here is the actual code that should have been updated to fix this problem instead of polluting cordova-android: https://github.com/apache/cordova-coho/blob/2ae50f2200c352f2a69b731a79f10562c5c41bc6/src/versionutil.js#L101-L122

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

Successfully merging this pull request may close these issues.

3 participants