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