0xced commented 8 years ago

The "Make the Mac Great Again" plugin would not install because Alcatraz assumes that the .xcodeproj has the exact same name as the package. This may not be true, for example "Make the Mac Great Again" xcodeproj file is "MakeTheMacGreatAgain.xcodeproj" (without the spaces).

This new heuristic searches for all .xcodeproj and works fine if only one exists in the cloned directory.

I also refactor error handling for better diagnostics when things go wrong.

0xced commented 8 years ago

Ha, I should have looked at the pull requests first, #461 implements pretty much the same heuristic! But this pull request adds better error messages in addition to this new heuristic.

guillaumealgis commented 8 years ago

Ha! It's really close to what I was doing with #461 indeed :)

461 was stalled because I didn't take the time to test that all packages (color schemes and templates included) still installed fine after the change. Did you test your implementation on all packages?

In particular, I'm a bit worried that in your case installNameFromPbxproj: did not change that much (in contrast to https://github.com/alcatraz/Alcatraz/pull/461/commits/efd7888ae00ec081888d70b81e8afbe583ff4d5d#diff-b22b9cdae9c200d1c9d03235e2928944L148). But then it's not fresh in my mind, so maybe this works fine 👍. My bad, did not see https://github.com/alcatraz/Alcatraz/pull/471/commits/e8390db498df4917a0847df1576ba2424f93d90b 🙂

Anyway, appart a few things (I'll comment inline), I'd be ok to merge your PR instead of #461 (if it's tested against all packages 😉).

0xced commented 8 years ago

I'd be ok to merge your PR instead of #461 (if it's tested against all packages 😉).

I’m not sure if you are serious or not. I don’t think testing against all packages will bring value. This PR is about getting more packages to install. If some are still failing to install after this PR is merged, we should handle the issues on a per package basis IMHO.

guillaumealgis commented 8 years ago

@0xced So sorry if I wasn't clear. I am serious, but the goal is to ensure that we don't break the install process for the plugins which are already working today.

If some are still failing to install after this PR is merged, we should handle the issues on a per package basis IMHO.

Agreed!

0xced commented 8 years ago

the goal is to ensure that we don't break the install process for the plugins which are already working today

I took care of keeping the current behaviour of returning the .xcodeproj as soon as found if it has the same name as the package, so the risk of breaking existing packages is extremely low.

supermarin commented 8 years ago

Thanks for the contribution @0xced! 🍺

👍 when @guillaume-algis is ok to merge

guillaumealgis commented 8 years ago

I hacked together an ugly script to install all packages on my machine. Will report tomorrow on the findings. Thanks for your patience @0xced!

jurre commented 8 years ago

If anything, it would be nice to make sure that we install more packages correctly this way

guillaumealgis commented 8 years ago

After merging this PR, the following packages will install correctly instead of failing:

Build Me Up
Fastlane-Plugin
Make the Mac Great Again
Refactor Code
XYZHappyCoding
ZLCheckFile

This does not break any plugin which previously installed, as expected 🎉

Full report here: https://gist.github.com/guillaume-algis/1da584f0964e94803056782394137e6e

One final thing before merging: I think this deserves a minor version number bump, not just a patch (I consider this to be more than just a bugfix), wdyt? 1.2.0?

jurre commented 8 years ago

Great! Looks like a good improvement then. I agree on the version bump.

👍

guillaumealgis commented 8 years ago

Hi @0xced, would you mind amending https://github.com/alcatraz/Alcatraz/pull/471/commits/47c562b70e35eccf59ac87feb3747e74260151ed with 1.2.0 instead of 1.1.19 and rebasing your branch onto master so we can merge this? 🍻 Thanks!

0xced commented 8 years ago

Done.

guillaumealgis commented 8 years ago

👍

Perfect, thank you!

guillaumealgis commented 8 years ago

@jurre @kattrali @supermarin what do you think? LGTM?

supermarin commented 8 years ago

👍

guillaumealgis commented 8 years ago

Thank you @supermarin!

@0xced do you mind rebasing on top of master once more? Thanks!

0xced commented 8 years ago

Sure, I just rebased.

guillaumealgis commented 8 years ago

:+1:

Ugh. We need to re-approve. @supermarin ?

Edit: And the comment-parsing by pullapprove doesn't seems to work for some reason. I approved manually on pullapprove.com 🤔

jurre commented 8 years ago

:+1:

guillaumealgis commented 8 years ago

Thanks @jurre ! 🙂

jurre commented 8 years ago

Thanks @0xced and @guillaumealgis :)

alcatraz / Alcatraz

Better heuristic for finding the .xcodeproj directory #471

461 was stalled because I didn't take the time to test that all packages (color schemes and templates included) still installed fine after the change. Did you test your implementation on all packages?