devtodo2 icon indicating copy to clipboard operation
devtodo2 copied to clipboard

Issue#9configfile

Open DenLilleMand opened this issue 8 years ago • 6 comments

Trying to fix #9 and #10 in one PR. Pretty much just putting this up here, its not a final version of the PR, but its a good foundation for something that could be merged i think.

It roughly contains:

  • Some refactoring, mostly changing strings hardcoded to constants in order to make it easier to test. Just adds more correctness aswell, naming conventions can be discussed though.
  • Support for .todorc incl. all options(excl. new color options, as discussed)
  • Tests of configuration and the order in which it loads
  • Changed ConsoleView to return strings instead of printing out directly, and wrote some tests to see if the ConsoleView prints out the correct bg and fg colors according to some viewOptions.
  • Some old commits that changed the Makefile, but then half way through i can see that i roll back the changes to a Makefile in another commit, that is already merged into this repository. Obviously that's not the best way to do it.
  • Adding new file config.go to Makefile

Currently known errors/bugs/not so great things:

  • Minor: Missing update of README, but i guess that should be done in the very end.

DenLilleMand avatar Apr 16 '17 22:04 DenLilleMand

Hi @alecthomas . I think the PR are getting closer to finished, let me know what you think whenever you have time. I was looking into how to squash the commits of a PR, but i don't think its possible without write access for the repo unfortunately. The commits are getting kind of messy.

DenLilleMand avatar Jun 15 '17 22:06 DenLilleMand

Don't worry about the number of commits. GitHub has the option to Squash and Merge now!

alecthomas avatar Jun 16 '17 03:06 alecthomas

Okay @alecthomas , so i changed the things you suggested. I think you're right about the cmdline options tbh it didn't make that much sense. And now kingpin doesn't have to be used inside of config.go either, which i think is better anyway. Makes the main.go file easier to understand when you can see the kingpin.parse() being called.

DenLilleMand avatar Jun 17 '17 22:06 DenLilleMand

Hey @alecthomas - Do you think i am missing something from the implementation before we can merge? . I am sorry about trying to talk on your behalf in that other issue, i just assumed that you we're on vacation or something since the replies on this PR stopped, that was obviously a misconception though :)

DenLilleMand avatar Jul 12 '17 16:07 DenLilleMand

Hi @DenLilleMand . I'm hesitating because I want to add configuration loading to Kingpin, and that would make most of this work redundant. I do appreciate the effort though, so I'm not sure what to do.

alecthomas avatar Jul 13 '17 04:07 alecthomas

Yeah alright np, its up to you of course :) its not completely wasted since i have a better understanding of the code base because of this work.

I don't know how the kingpin configuration API is going to look - but i would think that there is some value to merging still though because this PR has the tests covering the features discussed, although the tests are depending on the method loadCLIConfig, could be changed. But that would make it possible to easier refactor the current solution.

Some of the code is some refactoring of the consoleview aswell, so it returns strings instead of just printing out directly - which makes it testable.

DenLilleMand avatar Jul 13 '17 13:07 DenLilleMand