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

Added support for a config file #43

Merged
merged 4 commits into from Dec 30, 2015

Conversation

Projects
None yet
2 participants
@freiguy1
Contributor

freiguy1 commented Dec 4, 2015

This comes from a feature request in issue #37. The config file the code looks for now is config.yml instead of the suggested cobalt.yml in the issue. This was just an oversight on my part. I'm not terribly experienced in the github fork/PR lifecycle and rust itself, but I'm learning! Any positive/negative criticism is encouraged.

@johannhof

This comment has been minimized.

Show comment
Hide comment
@johannhof

johannhof Dec 6, 2015

Member

Hey @freiguy1, thank you very much for the pull request! I'm currently traveling and will only be able to take a look at it mid next week. First impression of this is 👍 , but I'd like to give it a closer look before merging.

Member

johannhof commented Dec 6, 2015

Hey @freiguy1, thank you very much for the pull request! I'm currently traveling and will only be able to take a look at it mid next week. First impression of this is 👍 , but I'd like to give it a closer look before merging.

@freiguy1

This comment has been minimized.

Show comment
Hide comment
@freiguy1

freiguy1 Dec 7, 2015

Contributor

Alright, no problem. A couple things since I've noticed since writing this:

  • The config.yml file gets copied into the target directory which it shouldn't.
  • At the moment the config.yml file needs to be in the current directory. There is no command line argument to change this. Also, you can set source directory in the config.yml. That means that your config.yml can live completely separately from your source directory. I think a better option would be to not have source set-able in config.yml - only in the command line arguments. Then the config file should exist in the source directory.

Perhaps this can be discussion for later.

Contributor

freiguy1 commented Dec 7, 2015

Alright, no problem. A couple things since I've noticed since writing this:

  • The config.yml file gets copied into the target directory which it shouldn't.
  • At the moment the config.yml file needs to be in the current directory. There is no command line argument to change this. Also, you can set source directory in the config.yml. That means that your config.yml can live completely separately from your source directory. I think a better option would be to not have source set-able in config.yml - only in the command line arguments. Then the config file should exist in the source directory.

Perhaps this can be discussion for later.

@johannhof

This comment has been minimized.

Show comment
Hide comment
@johannhof

johannhof Dec 16, 2015

Member

Finally got around looking at this, sorry.

I don't see anything wrong with setting src in the config.yml, I'd rather allow the cobalt executable to be passed a --config option, but that's for another PR.

One (cheap) way to solve the copying would be to make the file name.cobalt.yml (with a leading dot), which will be ignored by the generator.

We should go for .cobalt.yml instead of config.yml as default name, would you be up for changing that? :)

Member

johannhof commented Dec 16, 2015

Finally got around looking at this, sorry.

I don't see anything wrong with setting src in the config.yml, I'd rather allow the cobalt executable to be passed a --config option, but that's for another PR.

One (cheap) way to solve the copying would be to make the file name.cobalt.yml (with a leading dot), which will be ignored by the generator.

We should go for .cobalt.yml instead of config.yml as default name, would you be up for changing that? :)

@freiguy1

This comment has been minimized.

Show comment
Hide comment
@freiguy1

freiguy1 Dec 16, 2015

Contributor

Alright, I made that change. I'm not sure making the config a hidden file is appropriate, but it does get us up and running now. It seems inconsistent with how _layouts and _posts are named where they aren't hidden, but instead the program knows to avoid copying these key folders wholesale.

Contributor

freiguy1 commented Dec 16, 2015

Alright, I made that change. I'm not sure making the config a hidden file is appropriate, but it does get us up and running now. It seems inconsistent with how _layouts and _posts are named where they aren't hidden, but instead the program knows to avoid copying these key folders wholesale.

@johannhof

This comment has been minimized.

Show comment
Hide comment
@johannhof

johannhof Dec 30, 2015

Member

Yeah you make a good point about the inconsistency. I think it's still ok though, I consider _layouts and _posts to be actual content that is "transformed" and .config is just meta-information for the engine.

I guess the biggest problem is just implicitly copying all static content from the build folder into the target folder. Not sure if we want to keep that in the long run, it's just what has worked for me so far.

Member

johannhof commented Dec 30, 2015

Yeah you make a good point about the inconsistency. I think it's still ok though, I consider _layouts and _posts to be actual content that is "transformed" and .config is just meta-information for the engine.

I guess the biggest problem is just implicitly copying all static content from the build folder into the target folder. Not sure if we want to keep that in the long run, it's just what has worked for me so far.

@johannhof

This comment has been minimized.

Show comment
Hide comment
@johannhof

johannhof Dec 30, 2015

Member

Anyway, thanks for the contribution ❤️

Member

johannhof commented Dec 30, 2015

Anyway, thanks for the contribution ❤️

johannhof added a commit that referenced this pull request Dec 30, 2015

Merge pull request #43 from freiguy1/master
Added support for a config file

@johannhof johannhof merged commit e586ac6 into cobalt-org:master Dec 30, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 94.531%
Details
@johannhof

This comment has been minimized.

Show comment
Hide comment
@johannhof

johannhof Dec 30, 2015

Member

@freiguy1 After merging the PR I actually noticed two small things I wanted to change, you can check out the changes at e586ac6...b3e6dd4

Member

johannhof commented Dec 30, 2015

@freiguy1 After merging the PR I actually noticed two small things I wanted to change, you can check out the changes at e586ac6...b3e6dd4

@freiguy1

This comment has been minimized.

Show comment
Hide comment
@freiguy1

freiguy1 Dec 31, 2015

Contributor

Awesome, my pleasure! This is now my first PR and first merged PR. I'm hoping to contribute more in the future.

Contributor

freiguy1 commented Dec 31, 2015

Awesome, my pleasure! This is now my first PR and first merged PR. I'm hoping to contribute more in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment