|
|
|
go.talks/pkg/present: replace direct file system access with access through a ParserContext.
Patch Set 1 #Patch Set 2 : diff -r 2e320f363bb2 https://code.google.com/p/go.talks #Patch Set 3 : diff -r 2e320f363bb2 https://code.google.com/p/go.talks #
Total comments: 4
Patch Set 4 : diff -r 2e320f363bb2 https://code.google.com/p/go.talks #
Total comments: 3
Patch Set 5 : diff -r 2e320f363bb2 https://code.google.com/p/go.talks #
Total comments: 2
Patch Set 6 : diff -r 2e320f363bb2 https://code.google.com/p/go.talks #Patch Set 7 : diff -r 2e320f363bb2 https://code.google.com/p/go.talks #
Total messages: 14
|
gburd
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
|
12 years, 11 months ago (2013年02月10日 22:45:30 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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
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)
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.
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.
Hello adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
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
Hello adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
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
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.
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.
Hello adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
LGTM
*** 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>