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

move config code concerned with paths in to separate package #212

Merged
merged 1 commit into from
Sep 1, 2015

Conversation

lcowell
Copy link
Contributor

@lcowell lcowell commented Aug 30, 2015

I wanted to resolve issue #211 , but I quickly found that it was difficult to reason about how and when paths are calculated. This is an attempt to centralize the knowledge of paths in the exercism cli.

@Tonkpils
Copy link
Contributor

From looking at the paths package, it seems that all the functions in there still include Path as part of the name. This leads me to question if we actually need another package to handle this functionality or we can just place this functionality in a file called paths.go in the config package and place all that functionality there.

I know there's different views on when to separate packages but I thought it'd be good to discuss it. /cc @kytrinyx thoughts?

Other than that, this looks good and I agree this functionality needs to be grouped together.

I wanted to resolve issue exercism#211 , but I quickly found that it was
difficult to reason about how and when paths are calculated. This is an
attempt to centralize the knowledge of paths in the exercism cli.
@lcowell
Copy link
Contributor Author

lcowell commented Aug 30, 2015

Thanks @Tonkpils - I hadn't even thought of including it in config, but it's possible that it's premature packagization. What would be the advantage of keeping it all in the config package? BTW I've updated the names to be paths.Config() or paths.Exercises() to remove the stutter.

I'm not adamantly opposed to folding it back in to config, but it feels like a separate concern and gives us a nice bucket for other functions relating to paths like paths.IsDir(). I also like that it forces interactions between the areas of code to be explicit (which is how I was able to clean things up). I could also see this being a place where we describe how to construct other paths such as paths to language directories or assignment directories.

@kytrinyx
Copy link
Member

kytrinyx commented Sep 1, 2015

I'm a huge fan of tiny packages, and tend to prefer extracting if possible, then if later we find that it's really not worth the stand-alone package, we could easily fold it back in. My vote is for extracting the package now and then living with the decision for a while to see if we like it.

@Tonkpils
Copy link
Contributor

Tonkpils commented Sep 1, 2015

I'm fine with that. I'll go ahead and merge this

Tonkpils added a commit that referenced this pull request Sep 1, 2015
move config code concerned with paths in to separate package
@Tonkpils Tonkpils merged commit f58dd42 into exercism:master Sep 1, 2015
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.

3 participants