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

Remove CasperJS from the GeoExt repository only installing it via NPM if needed. #285

Merged
merged 6 commits into from
Sep 18, 2014

Conversation

bentrm
Copy link
Member

@bentrm bentrm commented Sep 8, 2014

These changes aimed to remove unrelated files from the GeoExt repository by removing the duplicated CasperJS repository. Readme.md, .travis.yml and .gitignore have been updated accordingly to make sure everything still works as expected.

I hope I'm not intruding on other build mechanics that are not apparent via the repo. If these changes are a no-go, I'm ok with it.

@marcjansen
Copy link
Member

I like this. I always wanted to remove the copied casperjs, but I am glad you finally did it.

This change does introduce npm as new dependency, but I think this is absolutely OK.

I'd love to hear a short sentence from @chrismayer whether everything works for him on his windows machine, but otherwise this looks great to me.

Thanks anyhow, I am sure that we'll merge this sooner or later.

Hey @bentrm, you may be interested to also have a look at #286 while you're looking at headless testing.

@bentrm
Copy link
Member Author

bentrm commented Sep 9, 2014

Good point, I kind of disregarded Windows. Though, I'm not sure if the headless tests have been working on Windows anyway. I just tried it with a fresh clone on a development machine. Seems like you have to run batchbin\casperjs instead of bin\casperjs. Still, no luck with a fresh branch nor this PRs branch.

@marcjansen
Copy link
Member

I am pretty sure they didn't really work on windows. Let's see if @chrismayer finds out something. npm should help at least with the installation, right?

I personally don't think this should block this, as the headless tests are an added value for CI mainly.

And I really want to remove all the doubled content (14k lines).

@chrismayer
Copy link
Contributor

No, it did not work on Windows before. So we do not break anything with this PR and I am +1 for merging this.
I had also some problems running the headless tests after merging this PR. Maybe we open up another issue but with minor priority.

Thanks for tackling this!

@marcjansen
Copy link
Member

Thanks @chrismayer & @bentrm.

marcjansen added a commit that referenced this pull request Sep 18, 2014
Remove CasperJS from the GeoExt repository only installing it via NPM if needed.
@marcjansen marcjansen merged commit f5f5c14 into geoext:master Sep 18, 2014
@marcjansen
Copy link
Member

11 additions and 14,766 deletions

Awesome 😏 👍

@bentrm bentrm deleted the travis branch September 18, 2014 07:59
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