code review 7312072: go.talks/pkg/present: replace direct file system access...

go.talks/pkg/present: replace direct file system access with access through a ParserContext.

[gburd, 2013-02-10 22:45:30]

Hello adg@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.talks

[adg, 2013-02-11 00:51:41]

https://codereview.appspot.com/7312072/diff/5001/pkg/present/parse.go File pkg/present/parse.go (right): https://codereview.appspot.com/7312072/diff/5001/pkg/present/parse.go#newcode216 pkg/present/parse.go:216: type ParseContext struct { I would prefer the name Context. https://codereview.appspot.com/7312072/diff/5001/pkg/present/parse.go#newcode221 pkg/present/parse.go:221: var DefaultParseContext = &ParseContext{ DefaultContext https://codereview.appspot.com/7312072/diff/5001/pkg/present/parse.go#newcode233 pkg/present/parse.go:233: // Parse parses the document in the file specified by name. using DefaultContext https://codereview.appspot.com/7312072/diff/5001/pkg/present/parse.go#newcode234 pkg/present/parse.go:234: func Parse(pc *ParseContext, r io.Reader, name string, mode ParseMode) (*Doc, error) { Let's keep this function signature as-is, add a method func (*Context) Parse(r io.Reader, name string, mode ParseMode) (*Doc, error) and then make this function defer directly to DefaultContext.Parse(r, name, mode)

[gburd, 2013-02-11 01:59:20]

I chose the name ParseContext because I think a rendering context is also needed. I will start a new thread to discuss rendering and come back to this after rendering is sorted out.

[adg, 2013-02-11 02:00:48]

On 11 February 2013 12:59, <gary.burd@gmail.com> wrote: > I chose the name ParseContext because I think a rendering context is > also needed. I will start a new thread to discuss rendering and come > back to this after rendering is sorted out. > Sounds good. 

[gburd, 2013-02-11 20:28:42]

Hello adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.

[adg, 2013-02-12 04:01:22]

https://codereview.appspot.com/7312072/diff/5009/pkg/present/parse.go File pkg/present/parse.go (right): https://codereview.appspot.com/7312072/diff/5009/pkg/present/parse.go#newcode254 pkg/present/parse.go:254: func Parse(r io.Reader, name string, mode ParseMode) (*Doc, error) { doc comment https://codereview.appspot.com/7312072/diff/5009/pkg/present/parse.go#newcode255 pkg/present/parse.go:255: ctx := Context{ReadFile: ioutil.ReadFile} no more DefaultContext? I kinda liked that. It has precedence in go/build

[gburd, 2013-02-12 16:34:36]

Hello adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.

[adg, 2013-02-12 21:32:48]

https://codereview.appspot.com/7312072/diff/7008/pkg/present/parse.go File pkg/present/parse.go (right): https://codereview.appspot.com/7312072/diff/7008/pkg/present/parse.go#newcode254 pkg/present/parse.go:254: // DefaultContext is the Context used by Parse. These two comments are circular. DefaultContext is a Context that uses the local file system. https://codereview.appspot.com/7312072/diff/7008/pkg/present/parse.go#newcode255 pkg/present/parse.go:255: var DefaultContext = Context{ReadFile: ioutil.ReadFile} &Context

[gburd, 2013-02-12 22:59:03]

https://codereview.appspot.com/7312072/diff/5009/pkg/present/parse.go File pkg/present/parse.go (right): https://codereview.appspot.com/7312072/diff/5009/pkg/present/parse.go#newcode255 pkg/present/parse.go:255: ctx := Context{ReadFile: ioutil.ReadFile} On 2013/02/12 04:01:22, adg wrote: > no more DefaultContext? I kinda liked that. It has precedence in go/build Does a DefaultContext make sense when rendering options are added? Different contexts are needed for slides and articles.

[adg, 2013-02-12 23:46:03]

On 2013/02/12 22:59:03, gburd wrote: > https://codereview.appspot.com/7312072/diff/5009/pkg/present/parse.go > File pkg/present/parse.go (right): > > https://codereview.appspot.com/7312072/diff/5009/pkg/present/parse.go#newcode255 > pkg/present/parse.go:255: ctx := Context{ReadFile: ioutil.ReadFile} > On 2013/02/12 04:01:22, adg wrote: > > no more DefaultContext? I kinda liked that. It has precedence in go/build > > Does a DefaultContext make sense when rendering options are added? Different > contexts are needed for slides and articles. Good point. Let's leave the DefaultContext out, for now.

[gburd, 2013-02-13 02:53:31]

Hello adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.

[adg, 2013-02-13 03:44:24]

LGTM

[adg, 2013-02-13 03:45:48]

*** Submitted as https://code.google.com/p/go/source/detail?r=2202e5a97fc5&repo=talks *** go.talks/pkg/present: access files through new Context type R=adg CC=golang-dev https://codereview.appspot.com/7312072 Committer: Andrew Gerrand <adg@golang.org>

[remyoudompheng, 2013-07-20 20:02:00]

R=close