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

add default script to template travis.yml #7138

Merged
merged 2 commits into from
Feb 15, 2019

Conversation

KCErb
Copy link
Contributor

@KCErb KCErb commented Dec 3, 2018

Based on some discussion here: crystal-lang/crystal-book#299 (comment)

Our docs recommend that folks add these lines to their travis.yml but it seems reasonable that the template could be shipped with them by default.

@jhass
Copy link
Member

jhass commented Dec 3, 2018

We could also update https://github.com/travis-ci/travis-build/blob/master/lib/travis/build/script/crystal.rb#L62 to include it instead, so if we have another recommendation in the future people will automatically get upgraded to it.

@asterite
Copy link
Member

asterite commented Dec 3, 2018

I don't think enforcing the formatter by default is a good idea. Many don't know about it or even don't like it. One thing is specs passing, if that fails it's bad, but if the formatting is wrong the program will still work fine.

@RX14
Copy link
Contributor

RX14 commented Dec 5, 2018

We impose a lot of opinionated stuff on users with crystal init anyway, if users want they can very easily delete the line from the generated travis.yml. Having it is a good idea because it encourages open source projects to think about things like standard source style which do matter. It's easy to forget setting up something like this even if you do want it.

@refi64
Copy link
Contributor

refi64 commented Dec 5, 2018

Idea: what if there are two different template choices? One would be a more sparse set-up (e.g. shard.yml, minimal Travis config), and the other would add "best practices"-type stuff like this.

@j8r
Copy link
Contributor

j8r commented Dec 7, 2018

There is a travis.yml template?! crystal init should be as minimal and standard as possible. Travis isn't a standard, it's a for-profit company. It doesn't have to be advantaged vs Circle, Jenkins, Gitlab or whatever in Crystal.

Moreover, not everybody will use a CI. This travis template shouldn't be in the compiler.

@j8r
Copy link
Contributor

j8r commented Dec 7, 2018

I even think that the crystal init will better live to shards init which already exists.

There is a shards specific template file, https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/tools/init/template/shard.yml.ecr

Even though shards is for now the defacto package manager, the crystal compiler should be agnostic.

To sum up:

  • the Crystal compiler itself souldn't know shards
  • shards, of course, know and depends of Crystal
  • move all project init with the templating out (in shards or projects like create-crystal-app)

@z64
Copy link
Contributor

z64 commented Dec 7, 2018

@j8r Agreed - Rust does cargo new for scaffolding and it makes sense for the same reasons vs rustc. It's less "out of the box" but is a better separation of concerns.

@bcardiff
Copy link
Member

I think we could include these changes but commented with a note to the user. Uncomment if you want to run formatter on the CI together with the specs.

I think that is an improvement, gives awareness of the formatter tool functionality to the users that want will want to use travis because the configuration is already there.

@bcardiff
Copy link
Member

As for the discussion of minimality, no scaffolding will satisfy everybody. I think @paulcsmith liked the idea of a cli that will be able to expand/generate scaffolding based on options. like yeoman. Having CI options there will probably be a good thing to have.

But I prefer to encourage at least some CI with no effort by the end user.
Adding all configuration for main CI seems too much.

@paulcsmith
Copy link
Contributor

Generator options would be cool, but maybe not worth the time and effort as the language is still young. In the meantime you may want to leave a comment so people can discover how to check formatting in CI easily. Here's what we do for a new Lucky project:

https://github.com/luckyframework/lucky_cli/blob/2f51bd4ba67435ea7cebf5d6e4d4b677ff38e0be/src/web_app_skeleton/.travis.yml.ecr#L21-L24

script:
  - crystal spec
  # Uncomment the next line if you'd like Travis to check code formatting
  # - crystal tool format spec src --check

This way it is off by default but it is easy to add and discover when you want it. I remember having to poke around quite a bit to figure this out the first time through

@straight-shoota
Copy link
Member

Adding a travis.yml config by default is already an opinionated choice. I'm sure it would be fine to have a formatter check by default as well. It's a good thing to enforce some minimal code style. It should be acceptable to every Crystal developer. That's why it's a part of the compiler and not an external tool.

@asterite
Copy link
Member

We could probably make crystal init ask these questions to the user with some opinionated defaults. That would also simplify it's interface, right now I find it a bit confusing that you have to pass crystal init lib|app name, I always do crystal init name and get an error. With a redesigned interface it will just be crystal init and then the program will start asking questions (name? app or lib? which CI, if any? want to check format in CI? etc.)

@paulcsmith
Copy link
Contributor

paulcsmith commented Dec 19, 2018 via email

@j8r
Copy link
Contributor

j8r commented Dec 19, 2018

Good idea, but please if someone plans to work on this, consider integrating outside the compiler. (e.g. shards)

Again, the compiler job is to deal with source code, only; not providing shiny opinionated projet management abilities.

@RX14
Copy link
Contributor

RX14 commented Dec 20, 2018

Let's just alter this PR to leave it commented

@KCErb
Copy link
Contributor Author

KCErb commented Jan 3, 2019

OK, looks like an acceptable path forward was established, so I just commented out the controversial lines to strike a balance between encouraging and enforcing usage of the compiler's spec and format capabilities in CI. Thanks for the constructive conversation everyone!

@j8r j8r mentioned this pull request Jan 3, 2019
@RX14
Copy link
Contributor

RX14 commented Jan 4, 2019

This needs a rebase? What's wrong with CI?

@sdogruyol
Copy link
Member

@KCErb a rebase and this should be good to 🚢

@KCErb KCErb force-pushed the add-to-default-travis branch from 8066532 to dabaafa Compare February 11, 2019 10:06
@KCErb
Copy link
Contributor Author

KCErb commented Feb 11, 2019

@sdogruyol I rebased locally and pushed (by force ... not sure which step I muffed to need that) back up so I believe this is in a good place now. We'll see how the tests do.

@straight-shoota
Copy link
Member

After a rebase, you always need to force push because the commits are now based on a different parent.

@KCErb
Copy link
Contributor Author

KCErb commented Feb 11, 2019

@straight-shoota thanks for confirming, that's how it looked to me but I'm sure I have a lot left to learn about git best practices!

@straight-shoota straight-shoota added this to the 0.28.0 milestone Feb 15, 2019
@sdogruyol sdogruyol merged commit 8c55b83 into crystal-lang:master Feb 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.