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

Added config variations #82

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

herdigiorgi
Copy link
Contributor

This allow that the user con choose different config files for the same project. You can see it explained in the documentation i changed in docs/config.md.

@PuercoPop
Copy link
Collaborator

Hi, first kudos for writing documentation on new features!

I amenable to this change with one variation. Instead of a variation name we should pass the pathname to the different configuration file. This would simplify the implementation to something along the lines of.

(defun load-config (&optional (repo-dir "") config-file)
  (with-open-file
   (in (or config-file
           (discover-config-path repo-dir))
       :external-format :utf-8)
   ...))

Minor nitpicks:

  • Compare characters using #'char=, not #'everyp
  • I'd prefer to keep the parameters to main as optional instead of keep.
  • If we keep using names instead of pathnames, it would be a wasted opportunity not to use the term flavors instead of variations.

Anyhow my 2c, @redline6561 What do you think?

@kingcons
Copy link
Collaborator

Definite kudos for documentation updates with new features. 🍰 🎉

The one thing I wonder is if this could be incorporated in the Coleslaw CLI / command line app work that you were doing @PuercoPop. Might be a more natural fit as just a parameter to a "build" tool. Granted that CLI work might not be ready for merge, not sure of status.

👍 on those nitpicks. I love the "Flavors" shoutout, did you buy one of those LispMs from the eBay auctions? ;)

On a more general/personal note, I've finally gotten a handle on my new job as a programming instructor here in the second semester. Am looking forward to being more active on the Coleslaw front in 2-3 weeks. Top task on my list is getting us a real test suite, maybe fiveam, maybe stefil, maybe fiasco. Then helping get the other open pull requests and stuff from forks merged in. 🍪

@arademaker
Copy link

If I understood right, stefil is decrecated and fiasco is its sucessor.

@PuercoPop
Copy link
Collaborator

@arademaker not quite. stefil is deprecated in favor of hu.dwim.stefil. Fiasco is a fork from stefil fork. Their differentiating feature is providing restarts for interactive debugging when a test fails.

As of late I've favored prove as it doesn't require any ceremony (declaring a suite or a fixture or a test package) in order to begin writing and running tests.

@redline6561 I'd wish :'(.

@herdigiorgi
Copy link
Contributor Author

Its no clear for me what i have to change to merge this. I can edit this for the next week because now i have some exams.

As @PuercoPop said, is better to use the path to a configuration file, and i think that nothing more than that, because in the configuration can be the repo dir. I think that this could be better than "variations" or "flavors". An then we will have only:

(defun load-config (config-file) ... )

A question.... do i have to close this commit? (:

@PuercoPop
Copy link
Collaborator

@DiGiorgi sorry for the late reply.
No need to close this pull request, just implement the amended changes in a new branch and push them to your PR branch, in this case master.

git checkout <last-commit-from-upstream> -b patch-2
*new commits go here changes*
git push -f <fork>* patch-2:master

*this is the name of the remote of your github repo of coleslaw.

if you need any help, you can hit me up on #lisp. Also no rush so it can be after your exams.

PuercoPop added a commit that referenced this pull request Jun 24, 2015
  - main takes an additional optional parameter to specify the
    configuration file

Related Issue #82
Coleslaw needs a `.coleslawrc` file to operate properly. That file is usually located at
$HOME/.coleslawrc but may also be placed in the blog repo itself.
Coleslaw needs a configuration file to operate properly. It can has any name and
is usally placed in the blog repo itself.
Copy link
Member

Choose a reason for hiding this comment

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

The default value should be mentioned/kept though.

Also typos, "usually" and "have" instead of "has".

@Ferada
Copy link
Member

Ferada commented Oct 14, 2016

I'm all for being able to choose a different configuration file. Can we perhaps allow users to use the :repo-dir option directly in the configuration file as a default value such that only loading that file is sufficient to get all paths and options ready?

@guicho271828
Copy link
Contributor

looking into this.

@dertuxmalwieder
Copy link
Contributor

looking into this.

Any update?

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.

7 participants