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

Improvement for Travis CI #95

Merged
merged 19 commits into from
Dec 24, 2017

Conversation

ilya-khadykin
Copy link
Contributor

@ilya-khadykin ilya-khadykin commented Oct 11, 2017

Resolves #89

I propose using Gradle as a build tool for the track. It might increase difficulty for the learner a bit, but it's more flexible solution in the long run. Plus in real world everyone uses it, so it might be worth getting used to it for learners as well
What do you think?

@kytrinyx, are you Ok with this?
I'm sorry to bother you with this, but looks like I've lost the context of what desire project structure is now.
Should we use .meta directory and generate docs only using configlet? Where to store reference solution?
Could you please point me to the docs to refresh my memory? 🚑

TODO:

  • - receive a green light with the proposal
  • - add subproject build for each exercise
  • - update existing docs for a new approach

@ilya-khadykin ilya-khadykin changed the title [WIP] Travis CI integration [WIP] Improvement of Travis CI Oct 11, 2017
@Dispader
Copy link
Contributor

Hey, @m-a-ge — I had the same thought a bit ago, but was swayed away from it by the discussion over in Pull #31 .

I will say that I like you made the build itself separate from the individual exercises in the track— very much nicer than what I was considering. If we did keep the @Grabs in the test code, they could be run independently of Gradle, to keep the ask for the learners low. (They can execute the specifications from the command line, or whatever environment.)

The focus on improving the Travis build here makes me very happy. If we do go this way, I'd love to add CodeNarc to our examples; to make our examples, well... exemplary. 😄

@ilya-khadykin
Copy link
Contributor Author

@Dispader thanks for the comment.
I agree that such approach complicates things for the learner in comparison with what we how now.
By means of using Gradle Wrapper we are not forcing them to install Gradle itself and dealing with environment variables etc, which is a good thing. But still, it's more complex.

Not sure how to keep @Grab with Gradle build though. Any ideas?

@ilya-khadykin
Copy link
Contributor Author

Looks like I have an idea how to do that.
I'll write a script which will prepare subproject exercise builds by creating required directory structure and copying/creating required files

@ilya-khadykin
Copy link
Contributor Author

ilya-khadykin commented Oct 15, 2017

I've implemented a script that sets up subproject builds for each exercise and updates main build accordingly. After that the main build is kicked off.
I've already encontered issues when our example solutions failed to pass tests 😒 - https://travis-ci.org/exercism/groovy/builds/288261416
I fixed it in 83c9092 but example implementation for gigasecond also failed - https://travis-ci.org/exercism/groovy/builds/288263846?utm_source=github_status&utm_medium=notification

I'll investigate this failures later

@Dispader I hope you like my new approach 💻

@ilya-khadykin
Copy link
Contributor Author

Ok, I fixed test Spec for gigasecond in b80fcbf
But I am not sure if it is a right fix, should investigate further

@ilya-khadykin ilya-khadykin changed the title [WIP] Improvement of Travis CI Improvement for Travis CI Oct 17, 2017
@ilya-khadykin
Copy link
Contributor Author

I've finally finished with this one and it's ready for review, build is passing.

@Dispader
@kytrinyx
Could you take a look?

P.S. the history a bit messed up, should fix it at some point

Copy link
Contributor Author

@ilya-khadykin ilya-khadykin left a comment

Choose a reason for hiding this comment

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

Left comments for myself to not forget

@@ -1,4 +1,3 @@
@Grab('org.spockframework:spock-core:1.0-groovy-2.4')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should revert this one

@@ -1,4 +1,3 @@
@Grab('org.spockframework:spock-core:1.0-groovy-2.4')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will revert this one

build.gradle Outdated
@@ -0,0 +1,57 @@
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should remove this default comments

@kytrinyx
Copy link
Member

kytrinyx commented Oct 22, 2017

@Dispader @m-a-ge I'm trying to catch up on a couple of weeks worth of notifications. I'll leave the decision about build tool to the two of you.

I've got some documentation about the goals of exercises here: https://github.com/exercism/docs/tree/master/language-tracks/exercises I'm not sure if that answers your questions.

I'd love to figure out what's missing so we can update that doc.

There are some really tricky trade-offs that I think we need to balance when choosing the technologies that an Exercism exercise requires/depends on.

  • It should be easy to get started; we want to avoid complicated dependencies that will make people confused, frustrated, etc before they even get started with their first exercise
  • It should not be magical; we don't want to hide the actual workings of the tooling
  • It should be idiomatic for the language; we want to use tooling and libraries that are standard in development for people who use the language professionally

It's not always possible to have all of those things.

We also want to avoid duplication where possible, but that's not a very strong requirement and we'll be adding some tooling to v2 to make it possible to have some global files for a track that get delivered to the user once, and that all the exercises can depend on.

@ilya-khadykin
Copy link
Contributor Author

@kytrinyx thanks for the explanation.

My first proposed solution complicated things (breaking the notion of "It should be easy to get started;").
But I've changed the approach of solving the initial problem and it'll be a good addition the to the track (I'm sure 😄)

Thanks for the link btw, it refreshed my memory.

I'll wait for @Dispader to take a look

@Dispader
Copy link
Contributor

@m-a-ge And, after nearly a month of getting swamped out by chaos and some health issues, I do plan to be back. Sorry for disappearing, there! Hope to have a look soon, but wanted you to know I hadn't forgotten about y'all.

@ilya-khadykin
Copy link
Contributor Author

@Dispader it's good to have back 👍

@ilya-khadykin ilya-khadykin mentioned this pull request Dec 21, 2017
11 tasks
@Raibaz
Copy link
Contributor

Raibaz commented Dec 22, 2017

Jumping in just to give an opinion on this :)

In my opinion, the decision to include gradle building or not in the exercises relates to the distinction between fluency and proficiency described in the "goal of exercism" doc.

In this case, being able to use gradle (or any other build tool) would be part of level 3 of proficiency, "Real-world programs - you can use the programming language to solve day-to-day problems using the tools and libraries in the broader ecosystem of the language.", which seems to be out of exercism's scope,

but

Java exercises are built with gradle, so we may want to use it as well for consistency.

That said, I was wondering: would it be possible/make sense to have the first, introductory, exercises be built without gradle (i.e., as they are now on master) and to introduce gradle builds from a certain step on?

@ilya-khadykin
Copy link
Contributor Author

@Raibaz I want to clarify a bit my approach to the problem.
The initial proposal included Groovy subproject builds for every exercise, but (after the feedback from other team members) I've changed it.

The current version doesn't temper with the files delivered to the user in any way. The Gradle subproject build is set up automatically by the script I've written https://github.com/exercism/groovy/pull/95/files#diff-0723ec0078ef82bfba7a671561ca8704 and it's used to run tests and make sure everything is Ok in a PR. User isn't forced to use Gradle, they are to continue using existing approach with Specs and Gradle is used only for our administration and maintenance purposes, see the build results in this PR - https://travis-ci.org/exercism/groovy/builds/288918038#L551

So, answering your question, there is no need to introduce Gradle and explain what it is and how to use it for our users.

What do you think now?

@Raibaz
Copy link
Contributor

Raibaz commented Dec 23, 2017

Oh ok, that makes a lot more sense to me then.

Sorry for mistaking your intent here, I totally agree with you this would be helpful :)

@ilya-khadykin ilya-khadykin merged commit 4dbc67f into exercism:master Dec 24, 2017
@ilya-khadykin
Copy link
Contributor Author

@Raibaz on this positive note I've merged this PR.
Thanks for your input!

@Raibaz
Copy link
Contributor

Raibaz commented Dec 27, 2017

I just have a minor concern about the linked-list exercise: when I was porting it from java, I had some issues since calling a class LinkedList may actually cause users (and the interpreter as well) confusion with the java.util.LinkedList class, which was why I renamed the class to DoubleLinkedList as in the Java version: here I see you renamed it back to LinkedList to comply with the exercise name.

I think it may make sense to rename the whole exercise :)

@ilya-khadykin
Copy link
Contributor Author

@Dispader,

I think it may make sense to rename the whole exercise :)

Yeah, probably. Would you mind creating an issue for this for further discussion?

@Dispader
Copy link
Contributor

Dispader commented Jan 6, 2018

Hey, @m-a-ge , I think it was @Raibaz that suggested the rename— and I agree that we should open another issue for it. (I'd lean towards it being renamed, but that might be a broader issue for general exercises.)

How are folks feeling about this Pull?— I'm thinking that it's an improvement.

@Raibaz
Copy link
Contributor

Raibaz commented Jan 6, 2018

I already created both the issue and the PR for the rename, and i think it's also already been merged :)

@ilya-khadykin
Copy link
Contributor Author

ilya-khadykin commented Jan 9, 2018

@Dispader I think it's a good thing to rename that exercise since there is an inconsistency in its description and its name (even in problem-specification)

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.

4 participants