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

Alternative Travis build #71

Merged
merged 1 commit into from
Sep 28, 2017
Merged

Alternative Travis build #71

merged 1 commit into from
Sep 28, 2017

Conversation

icyrockcom
Copy link
Contributor

Work in progress PR, please do not merge this yet, see below for more (too much?) details.

This is an alternative Travis build setup. Here's what I changed:

  • All exercises are compiled and tested in one run
  • Travis cache now has only the bower cache
  • Updated many versions (both build tools and actual purescript libraries)
  • Extracted common bower.json to etc/bower.json with dependencies from all exercises
  • Updated the tests to remove compilation warnings due to missing / incomplete signatures
  • Tests were also split into main and suites functions to help with running the tests
  • Small update to Bob exercise which was failing due to recent test case updates

Sample build that demonstrates how it works:

https://travis-ci.org/icyrockcom/xpurescript/builds/278841675

The build is failing due to linter failing on:

The following UUID was found in multiple Exercism tracks. Each exercise UUID must be unique across tracks. c41a2961-f7ca-4eaf-83cf-48762fc39c06

but otherwise goes fine (finished in ~18 minutes). Fixing the linter issue would make this a good build.

Can someone let me know if this is just updating the UUIDs? Easy to do, but I was wary after finding an issue which says something to the effect of "this will delete the exercise and all its history" (exercism/discussions#159 (comment)).

I have the following additional concerns:

  • Shared bower.json has to be used going forward (not sure if this is a positive or a negative)
  • Each exercise now has to have exactly one module in examples/src and test folders
  • This is slightly hackish, as it requires collecting and modifying the test sources (to change the module from Test.Main to Test.ExerciseName), is that acceptable from your standpoint?
  • Obviously there's a possibility of interference across exercises (though previous build also shared the same bower dependencies, so had that to some degree)

If this approach is fine with you guys, I would like to polish it a bit more:

  • Fix each of the exercises (it should be only copying the bower.json, but need to confirm)
  • Add dependency checker to the build which would confirm that all exercises are using the same versions as the common bower.json
  • Add a script to copy the common bower.json to all exercises to help with the development
  • Update documentation for contributing to mention the additional restrictions

Let me know what you think, if you have additional comments / concerns. Most importantly, if I should go ahead in this direction and with the above changes.

@icyrockcom
Copy link
Contributor Author

OK, UUID issue looks like it's due to my repository being still named xpurescript (i.e. I still have x upfront). I guess that can be ignored, since the main build passes fine.

Let me know about the other things.

@icyrockcom
Copy link
Contributor Author

@paf31 @lpil Any comments on the above approach?

@lpil
Copy link
Member

lpil commented Sep 28, 2017

Hey @icyrockcom

Seems sensible to me. My only gripe is the lack of semicolons in your Javascript ;)

@icyrockcom
Copy link
Contributor Author

Thanks @lpil! I'll work on polishing the PR then.

The semicolons, I actually prefer not having them. Most of the time, they just are syntactic noise, don't you agree? I'll add them to this PR, not a problem.

@paf31 Let me know if you have any comments from your end.

@paf31
Copy link
Contributor

paf31 commented Sep 28, 2017

Sorry I don't have time to review right now, but if @lpil is happy, I say ship it.

@icyrockcom
Copy link
Contributor Author

No worries, thanks @paf31! We can revisit later if needed.

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

I'm just teasing with the semicolons. Old habits die hard. I use prettier so I rarely type them myself.

Anyway, lets merge this :)

@lpil lpil merged commit 094d7f6 into exercism:master Sep 28, 2017
@icyrockcom
Copy link
Contributor Author

@lpil "Old habits die hard." Haha, true that! :) OK, thanks for merging. I'll do the rest in another PR.

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