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

Omniconf implementation #5

Closed
wants to merge 10 commits into from
Closed

Omniconf implementation #5

wants to merge 10 commits into from

Conversation

RandomGuyOnTheInternet1234
Copy link
Contributor

This pull request implements omniconf configuration to store variables about where the git repos are located. Also, I fixed the bug where I couldn't run the code in a directory with a space.

jakeinc added 8 commits January 26, 2017 18:28
@dgopstein
Copy link
Owner

Thanks for the PR, it's looking pretty good! Some comments:

  • I don't know if you saw but this change has "merge conflicts" which means that the version of the code you were working from got changed in the meantime and so git doesn't know how to apply your changes anymore. In order to fix this you need to do a git mergetool. Sometimes this can be a real hassle, but in this case I think the conflicts are pretty minor.

  • Instead of needing to import omniconf.core, and remember the name of the conf file and the settings in that conf file for every access, can you just use your conf code to populate the values in constants.clj one time? And leave the references to gcc-path, the way they were in the code before. This should end up being a much smaller diff, and make it much easier to use these values in the future.

  • Don't put :jake-dev-conf in the codebase since me and Henry and anyone else will want our own settings as well. Instead the code should be perfectly generic, and the only thing that changes from my machine to yours is which conf file we use. I can have dan.edn and you can have jake.edn, but inside they all have the same structure with different variables. And from there we can each symlink our own conf to the general conf.edn. Or something.

  • Probably the conf files should live in src/conf or something instead of at the root level, since there will likely need to be a bunch of them.

@@ -16,6 +16,7 @@
union.c
wdate-time.c)]
(doseq [filename files]
(println (resource-path filename))
Copy link
Owner

Choose a reason for hiding this comment

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

Can you remove this?

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.

2 participants