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

Content, layout config opt made available via command line #1698

Closed
wants to merge 1 commit into from

Conversation

kujohn
Copy link
Contributor

@kujohn kujohn commented Dec 10, 2015

I want to get some feedback before working further.
fixes #1598

@kujohn
Copy link
Contributor Author

kujohn commented Dec 10, 2015

Is this okay or should I instead add an additional argument to helper.AbsPathify for opt in of using WorkingDir? There's a todo comment here.

@spf13
Copy link
Contributor

spf13 commented Dec 24, 2015

Please don't add an additional argument. The current approach you are taking is sound.

I would rename your function to something else. It's not simpler than AbsPathify as it takes an additional argument.

@spf13
Copy link
Contributor

spf13 commented Jan 6, 2016

when you have this ready, please rebase from master and force push.

@kujohn
Copy link
Contributor Author

kujohn commented Jan 6, 2016

👍 ok

@kujohn kujohn force-pushed the devel/1598 branch 3 times, most recently from 431f075 to 2752d9f Compare January 8, 2016 05:31
@kujohn
Copy link
Contributor Author

kujohn commented Jan 8, 2016

Squashed and rebased, this should be good to go after code review.

@kujohn kujohn changed the title content, layout config opt made available via command line Content, layout config opt made available via command line Jan 8, 2016
@bep
Copy link
Member

bep commented Mar 10, 2016

@kujohn could you rebase this against master ... again; sorry for that :-)

@kujohn kujohn force-pushed the devel/1598 branch 2 times, most recently from 2ced457 to c5b51ab Compare March 10, 2016 15:11
@kujohn
Copy link
Contributor Author

kujohn commented Mar 10, 2016

Had some issue rebasing. Be back in an hour. 😭

@kujohn kujohn force-pushed the devel/1598 branch 4 times, most recently from 9d4140e to f8f32a4 Compare March 10, 2016 17:44
@kujohn
Copy link
Contributor Author

kujohn commented Mar 10, 2016

Rebased correctly I think.

var layoutPath string

if layoutDir != "" {
layoutPath = helpers.AbsPathifySimple(viper.GetString("LayoutDir"))
Copy link
Member

Choose a reason for hiding this comment

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

Why AbsPathifySimple? These new dir flags should behave exactly as the others. If there is some issue with AbsPathify, then that should be addressed in a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the working directory is set, this allows relative layout/content path to be set independently and working.
Perhaps this feature is not needed yet or that this func needs to be renamed?

@bep
Copy link
Member

bep commented Mar 11, 2016

Perhaps this feature is not needed yet or that this func needs to be renamed?

This is another issue not relevant to this issue. Remove that function and use the existing.

@kujohn kujohn force-pushed the devel/1598 branch 3 times, most recently from c54e34b to 92a0314 Compare March 11, 2016 16:36
@kujohn
Copy link
Contributor Author

kujohn commented Mar 11, 2016

ok squashed and cleaned up.

@moorereason
Copy link
Contributor

lgtm

@bep
Copy link
Member

bep commented Mar 11, 2016

Looks good to me, too, thanks. Merged in 0ab4162

@bep bep closed this Mar 11, 2016
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All location config options should be available via command line flags
4 participants