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

CB-14038 (android): fix false positive detecting project type #437

Merged
merged 1 commit into from
Apr 19, 2018

Conversation

jcesarmobile
Copy link
Member

Platforms affected

Android

What does this PR do?

Make the Android Studio check return true
Removed old android project structure tests and files

What testing has been done on this change?

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

@codecov-io
Copy link

Codecov Report

Merging #437 into master will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #437      +/-   ##
==========================================
+ Coverage   44.02%   44.16%   +0.14%     
==========================================
  Files          17       17              
  Lines        1715     1698      -17     
  Branches      319      314       -5     
==========================================
- Hits          755      750       -5     
+ Misses        960      948      -12
Impacted Files Coverage Δ
bin/templates/cordova/lib/AndroidStudio.js 100% <100%> (+5.26%) ⬆️
bin/templates/cordova/lib/AndroidProject.js 42.6% <0%> (+1.73%) ⬆️
bin/templates/cordova/Api.js 49.57% <0%> (+7.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76180d3...fe7629e. Read the comment docs.

@infil00p
Copy link
Member

infil00p commented Apr 18, 2018

It's probably safe to delete GradleBuilder.js since it should never run now that the isAndroidStudio is always true. Other than that, this looks fine.

@jcesarmobile
Copy link
Member Author

Yeah, but if I remove the file I should also remove the tests related to the builder, and as it's always true I can remove all the checks, but wanted to keep it simple as a fast fix and do the major refactor in the future

@macdonst
Copy link
Member

Wouldn't it make more sense to delete AndroidStudio.js as it is only used in a couple of places.

Then remove the if statement from here:

if (AndroidStudio.isAndroidStudioProject(projectDir) === true) {

and only set the www directory once to the AndroidStudio path.

Then move all the assignments in this if statement:

if (AndroidStudio.isAndroidStudioProject(self.root) === true) {

to the lines above so they are the defaults.

@infil00p
Copy link
Member

ZOMG we're dogshedding!

@jcesarmobile
Copy link
Member Author

As I said, my idea was to just add a quick simple fix to avoid the false positives just by setting isAndroidStudioProject to true, but in the end it wasn't as simple as I had to delete tests, and update some test to test the Android Studio case, and then delete the sample Eclipse project.
I can continue deleting code that is now irrelevant, which will lead to delete more test, and to delete more code again, but then it won't be quick nor simple.
There are also a lot of options.android_studio === true that are also irrelevant, and I could just delete that option as it's always true, and then I'll have to modify the tests to not use that option as it won't exist.

I think that kind of refactor can wait for a future release

@infil00p infil00p merged commit 59e3b90 into apache:master Apr 19, 2018
@jcesarmobile jcesarmobile deleted the CB-14038 branch April 20, 2018 09:29
@raphinesse
Copy link
Contributor

@jcesarmobile Is there any WIP for the aforementioned refactor? Losing some dead weight would definitely be good. Furthermore, before I discovered this PR by accident, I already thought about starting to DRY the two very similar Builders, not knowing that removal of one of them is due.

BTW: GradleBuilder is used by the Java unit test runner. Just to create a Gradle wrapper, so the dependency could be easily removed.

@jcesarmobile
Copy link
Member Author

No, there is no work about it because the idea was to do a patch or minor release with this changes to unblock people with plugin problems, and the refactor was supposed to be done for next major release. But right now it's not clear to me if we are going to do a patch, minor or major release.

Anyway, the refactor is not a breaking change, so it can be done at any time.

@raphinesse
Copy link
Contributor

Thanks for your answer.

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.

None yet

5 participants