Return filename from UseTempLogFile()

[mohae]

This is related to a comment that @owenwaller left in his updated hugo/utils_test.go: gohugoio/hugo#818

Is modifying UseTempLogFile() so that it returns the temp filename a change you would support? If so I can make a pull request.

[owenwaller]

My gut feel would be this....

If jww created the file - as is the case of UseTempLogFile() or
SetLogFilePath() then jww is responsible for that file. So jww
should ensure that the file is properly closed.

If jww is passed a *File to use as the log file (if this is indeed
the case) then jww is not responsible for the *File its the callers
responsibility, and they must Close() it.

In terms of returning the filename form UseTempFileName yes, I'd
support it :) It would simplify the test in question - gohugoio/hugo#818

But there's also I few other minor changes that I can think off as well

• such as removing the fmt.Printf's in the package or at least being
  able to turn them off.

So if you want to generate a pull request for the file name change, I
can build on that to do the other minor changes,

Owen

On Thu, 2015-02-19 at 15:39 -0800, Joel Scoble wrote:

This is related to a comment that @owenwaller left in his updated
hugo/utils_test.go: gohugoio/hugo#818

Is modifying UseTempLogFile() so that it returns the temp filename a
change you would support? If so I can make a pull request.

—
Reply to this email directly or view it on GitHub.

Dr. Owen Waller
Head of Development
Kulawe Limited
Forsyth House
Cormac Square
Belfast
Northern Ireland
BT2 8LA

+44 (0)757 882 9933
[email protected]
www.kulawe.com

Company Number: NI604010
VAT Number : GB 122 6319 42

[mohae]

After investigating this further, I'm not sure this is really needed. If the name of the file created by TempLogFile() is needed, it can be obtained by:

LogHandle.(*os.File).Name()

While it may be useful to a user of jww to have the created filename returned, it is something that can be obtained with another line of code and some assertion.

This is a decision I will leave to Steve. I this is something that should be supported I will make a pull request, otherwise this issue can be closed.

It should be noted that such a change would not affect existing code.

[owenwaller]

I agree. Given that we can access the file handle via the exported LogHandle variable then I don't see any point in returning it.

But looking at the public interface again though it still doesn't feel right.

We have both the SetLogFile() and UseTempLogFile() which are basically "Open" or "Create" functions. But there's no corresponding Close() function. We are expecting the user to remember to do this, and to do it via the exported LogHandle. There is the DiscardLogging function which could be mistaken for a Close() function, but it isn't and that's not its intent.

Maybe the simplest thing we can do is just to add a Close() function. At least then it is obvious to a user of the package that they need to so something to stop the logger cleanly.

[mohae]

Well just adding a Close() function isn't as simple as one may think. First the LogHandle must get something else, even if it is just an ioutuil.Discard. Also if you add Close() then the SetLogFile() and UseTempLogFile() need to be modified. It starts getting a little complex and ugly.

I started thinking about this for the Hugo issue that spawned this, until asserting the LogHandle interface to access the *os.File's Close() method was used, and I wasn't satisfied with any of the implementations that I came up with. I didn't spend very long on it though.

As jww is currently written, I'm not sure there is a pressing need for Close(). I can think of one scenario where it might be useful: when logging to a temp file on application start-up and then switching to the configured application log file once the ocnfiguration has been read, but that adds its own issues and I'm not sure about supporting that scenario.

I have been toying with the log flow described above and haven't been satisfied with anything I've done. I've been thinking about adding it to a custom branch of jww, but haven't gotten beyond contemplating it.

[bep]

I will soon remove this method and replace it with a new method that takes an io.Writer.