Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1574353004:
GitHub #8: Seed indexes from index.yml (Closed)
|
Created:
4 years, 11 months ago by nishanths (utexas) Modified:
4 years, 10 months ago CC:
chromium-reviews, infra-reviews+luci-gae_chromium.org Base URL:
https://github.com/luci/gae@master Target Ref:
refs/heads/master Project:
luci-gae Visibility:
Public. |
DescriptionFixes #8: Seed indexes from index.yml
- Add `ParseIndexYAML` and `FindAndParseIndexYAML` functions
to service/datastore
- Add YAML marshalling and unmarshalling for IndexDefinition and IndexColumn
- Add tests for the changes
- Imports: additional package: gopkg.in/yaml.v2
https://github.com/luci/gae/issues/8
Closes #8
R=dnj@chromium.org, estaab@chromium.org, iannucci@chromium.org, vadimsh@chromium.org
BUG=
Committed: https://github.com/luci/gae/commit/1fb004af6ec3634b29cd97406e17d4bb0a7d2236
Patch Set 1 #
Total comments: 23
Patch Set 2 : Address review comments: more tests + moving functions out of Testable + better style #Patch Set 3 : Fix missing parallel call in test #
Total comments: 1
Patch Set 4 : Use runtime.Callers instead of runtime.Caller #
Total comments: 4
Patch Set 5 : Improve loop style + Edit doc comments #Patch Set 6 : path argument in FindAndParseIndexYAML, add a couple of tests #
Messages
Total messages: 24 (5 generated)
On 2016/01/13 08:53:06, nishanths wrote: Potential feedback areas: - `panic` calls in FindAndAddIndexYAML and ParseIndexYAML - new imported package: gopkg.in/yaml.v2
Looks mostly good! The YAML import is fine. https://chromiumcodereview.appspot.com/1574353004/diff/1/impl/memory/datastor... File impl/memory/datastore.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/1/impl/memory/datastor... impl/memory/datastore.go:216: if fileExists(filepath.Join(dir, "index.yml")) { nit: Consider making this loop over a slice of candidate names: for _, name := range []string{"index.yml", "index.yaml"}{ { } You also don't need "fileExists", since "os.Open" naturally tests for existence. Just try to open each candidate file until one succeeds. https://chromiumcodereview.appspot.com/1574353004/diff/1/impl/memory/datastor... impl/memory/datastore.go:240: if dir == "/" && filepath.Dir(dir) == "/" { This is not a cross-platform check, as on Windows "Dir" will terminate with "X:\". You need to check if "dir" ends with os.PathSeparator. https://chromiumcodereview.appspot.com/1574353004/diff/1/impl/memory/datastor... File impl/memory/datastore_test.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/1/impl/memory/datastor... impl/memory/datastore_test.go:606: }) Also test filesystem walking using a tempdir. https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/in... File service/datastore/index_test.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/in... service/datastore/index_test.go:70: " kind: Kind", You should be able to avoid repeating "yaml" by Marshalling a []*IndexDefinition instead of an *IndexDefinition. https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/te... File service/datastore/testable.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/te... service/datastore/testable.go:93: FindAndAddIndexYAML() I don't think this capability needs to be added to the "Testable" interface. Maybe top-level methods in "impl/memory"? Also, both of these should return errors rather than panic on failure. They are explicitly called by the user, so it's reasonable to task the user with handling their failure.
Very cool! Some addon comments to what dnj said. https://chromiumcodereview.appspot.com/1574353004/diff/1/impl/memory/datastor... File impl/memory/datastore.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/1/impl/memory/datastor... impl/memory/datastore.go:210: panic(fmt.Errorf("failed to find index YAML file")) this error should be changed to be more meaningful ("failed to find source for test") In fact, the Caller(1) should probably be Caller(2)... We want to search from the location of the /test/ file that called FindAndAddIndexYAML. If you want to be fancy, you could walk up the call stack until you found the first file whose name ends with _test.go. https://chromiumcodereview.appspot.com/1574353004/diff/1/impl/memory/datastor... impl/memory/datastore.go:216: if fileExists(filepath.Join(dir, "index.yml")) { On 2016/01/13 at 16:16:40, dnj wrote: > nit: Consider making this loop over a slice of candidate names: > for _, name := range []string{"index.yml", "index.yaml"}{ > { > > } > > You also don't need "fileExists", since "os.Open" naturally tests for existence. Just try to open each candidate file until one succeeds. Moreover, doing os.Open directly avoids an (admittedly unlikely) race condition where the file could exist for the `fileExists` check, but not for the os.Open call. https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/in... File service/datastore/index.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/in... service/datastore/index.go:55: func (i *IndexColumn) UnmarshalYAML(unmarshal func(interface{}) error) error { I would consider making the Property and Descending fields be their own types which independently implement the *marshalYAML functions. That said, if it requires you to start changing oodles of files then never mind :). https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/in... service/datastore/index.go:62: i.Property = m["name"] should return an error if there's somehow no name https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/in... File service/datastore/index_test.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/in... service/datastore/index_test.go:24: serialized []string // without the leading hyphen, for instance I don't understand this comment https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/te... File service/datastore/testable.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/te... service/datastore/testable.go:7: import "io" always use import ( "io" ) style. It's easier to add stuff later. https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/te... service/datastore/testable.go:93: FindAndAddIndexYAML() On 2016/01/13 at 16:16:40, dnj wrote: > I don't think this capability needs to be added to the "Testable" interface. Maybe top-level methods in "impl/memory"? If anything they should be top level methods in service/datastore; there's nothing impl/memory-specific about them. To that end, moving them to be toplevel functions in this package: func ParseIndexYAML(io.Reader) ([]*IndexDefinition, error) func FindAndParseIndexYAML() ([]*IndexDefinition, error) would be fine. > > Also, both of these should return errors rather than panic on failure. They are explicitly called by the user, so it's reasonable to task the user with handling their failure. If they're in Testable, I disagree. 100% of the usecases would be to call them and then assert no error. If they're top level functions and not testing functions, then yes they should return errors.
https://chromiumcodereview.appspot.com/1574353004/diff/1/impl/memory/datastor... File impl/memory/datastore.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/1/impl/memory/datastor... impl/memory/datastore.go:210: panic(fmt.Errorf("failed to find index YAML file")) On 2016/01/13 19:12:20, iannucci wrote: > this error should be changed to be more meaningful ("failed to find source for > test") > > In fact, the Caller(1) should probably be Caller(2)... We want to search from > the location of the /test/ file that called FindAndAddIndexYAML. If you want to > be fancy, you could walk up the call stack until you found the first file whose > name ends with _test.go. Done. Also implemented "look until you find _test.go". As for the argument to runtime.Caller, if I'm not mistaken, according to the docs, 0 identifies the caller of runtime.Caller. https://golang.org/pkg/runtime/#Caller. So 1 would be anything higher up. The latest commit starts the loop in the `getCallingTestFilePath` function at 1, so that it's easier if/when `getCallingTestFilePath` is exported in the future. https://chromiumcodereview.appspot.com/1574353004/diff/1/impl/memory/datastor... impl/memory/datastore.go:216: if fileExists(filepath.Join(dir, "index.yml")) { On 2016/01/13 16:16:40, dnj wrote: > nit: Consider making this loop over a slice of candidate names: > for _, name := range []string{"index.yml", "index.yaml"}{ > { > > } > > You also don't need "fileExists", since "os.Open" naturally tests for existence. > Just try to open each candidate file until one succeeds. Done. https://chromiumcodereview.appspot.com/1574353004/diff/1/impl/memory/datastor... impl/memory/datastore.go:216: if fileExists(filepath.Join(dir, "index.yml")) { On 2016/01/13 19:12:20, iannucci wrote: > On 2016/01/13 at 16:16:40, dnj wrote: > > nit: Consider making this loop over a slice of candidate names: > > for _, name := range []string{"index.yml", "index.yaml"}{ > > { > > > > } > > > > You also don't need "fileExists", since "os.Open" naturally tests for > existence. Just try to open each candidate file until one succeeds. > > Moreover, doing os.Open directly avoids an (admittedly unlikely) race condition > where the file could exist for the `fileExists` check, but not for the os.Open > call. Thanks for the tip! https://chromiumcodereview.appspot.com/1574353004/diff/1/impl/memory/datastor... impl/memory/datastore.go:240: if dir == "/" && filepath.Dir(dir) == "/" { On 2016/01/13 16:16:40, dnj wrote: > This is not a cross-platform check, as on Windows "Dir" will terminate with > "X:\". You need to check if "dir" ends with os.PathSeparator. Done. https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/in... File service/datastore/index.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/in... service/datastore/index.go:55: func (i *IndexColumn) UnmarshalYAML(unmarshal func(interface{}) error) error { On 2016/01/13 19:12:20, iannucci wrote: > I would consider making the Property and Descending fields be their own types > which independently implement the *marshalYAML functions. That said, if it > requires you to start changing oodles of files then never mind :). Good idea. Will create a separate issue for these changes. :) https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/in... service/datastore/index.go:62: i.Property = m["name"] On 2016/01/13 19:12:20, iannucci wrote: > should return an error if there's somehow no name Done. https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/in... File service/datastore/index_test.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/in... service/datastore/index_test.go:24: serialized []string // without the leading hyphen, for instance On 2016/01/13 19:12:20, iannucci wrote: > I don't understand this comment My bad. Meant: not inside an array (YAML arrays have the leading hyphen)". The struct field and comment have both now been removed. https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/in... service/datastore/index_test.go:70: " kind: Kind", On 2016/01/13 16:16:40, dnj wrote: > You should be able to avoid repeating "yaml" by Marshalling a []*IndexDefinition > instead of an *IndexDefinition. Thanks! Much cleaner. :) https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/te... File service/datastore/testable.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/te... service/datastore/testable.go:7: import "io" On 2016/01/13 19:12:20, iannucci wrote: > always use > > import ( > "io" > ) > > style. It's easier to add stuff later. Done. https://chromiumcodereview.appspot.com/1574353004/diff/1/service/datastore/te... service/datastore/testable.go:93: FindAndAddIndexYAML() On 2016/01/13 19:12:20, iannucci wrote: > On 2016/01/13 at 16:16:40, dnj wrote: > > I don't think this capability needs to be added to the "Testable" interface. > Maybe top-level methods in "impl/memory"? > > If anything they should be top level methods in service/datastore; there's > nothing impl/memory-specific about them. > > To that end, moving them to be toplevel functions in this package: > > func ParseIndexYAML(io.Reader) ([]*IndexDefinition, error) > func FindAndParseIndexYAML() ([]*IndexDefinition, error) > > would be fine. > > > > > Also, both of these should return errors rather than panic on failure. They > are explicitly called by the user, so it's reasonable to task the user with > handling their failure. > > If they're in Testable, I disagree. 100% of the usecases would be to call them > and then assert no error. If they're top level functions and not testing > functions, then yes they should return errors. Done. Moved to service/datastore with the method signatures that @iannucci posted.
Please let me know if anything needs to be improved! https://chromiumcodereview.appspot.com/1574353004/diff/1/impl/memory/datastor... File impl/memory/datastore_test.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/1/impl/memory/datastor... impl/memory/datastore_test.go:606: }) On 2016/01/13 16:16:40, dnj wrote: > Also test filesystem walking using a tempdir. Done. :) The tests are now in service/datastore/datastore_test.go.
Description was changed from ========== GitHub #8: Seed indexes from index.yml - Add `ParseIndexYAML` and `FindAndAddIndexYAML` functions to Testable interface - Implement new functions for memory.dsImpl - Add YAML marshaling and unmarshaling datastore - Add a few tests for the changes - Imports: additional package: gopkg.in/yaml.v2 R=dnj@chromium.org, estaab@chromium.org, iannucci@chromium.org, vadimsh@chromium.org BUG= ========== to ========== GitHub #8: Seed indexes from index.yml - Add `ParseIndexYAML` and `FindAndParseIndexYAML` functions to service/datastore - Add YAML marshalling and unmarshalling for IndexDefinition and IndexColumn - Add tests for the changes - Imports: additional package: gopkg.in/yaml.v2 R=dnj@chromium.org, estaab@chromium.org, iannucci@chromium.org, vadimsh@chromium.org BUG= ==========
lgtm :) Thanks again! https://chromiumcodereview.appspot.com/1574353004/diff/40001/service/datastor... File service/datastore/datastore.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/40001/service/datastor... service/datastore/datastore.go:384: _, path, _, ok := runtime.Caller(skip) consider using https://golang.org/pkg/runtime/#Callers instead. Caller is essentially repeatedly calling Callers internally I think.
On 2016/01/15 03:06:08, iannucci wrote: > lgtm :) Thanks again! > > https://chromiumcodereview.appspot.com/1574353004/diff/40001/service/datastor... > File service/datastore/datastore.go (right): > > https://chromiumcodereview.appspot.com/1574353004/diff/40001/service/datastor... > service/datastore/datastore.go:384: _, path, _, ok := runtime.Caller(skip) > consider using https://golang.org/pkg/runtime/#Callers instead. Caller is > essentially repeatedly calling Callers internally I think. You're welcome, and thanks for the review :) I've updated to use `runtime.Callers`, which looks much neater. Thoughts on the maxStackDepth of 100 used? Besides that, I'm ready to merge whenever you are ready.
https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor... File service/datastore/datastore.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor... service/datastore/datastore.go:385: n := runtime.Callers(0, pc) try for _, elem := range pc[:runtime.Callers(0, pc)] { } https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor... service/datastore/datastore.go:406: path, err := getCallingTestFilePath(100) document the 100 element limit in the docstring above "If you have more than 100 stack frames between here and your test file, you're doing it wrong" :)
https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor... File service/datastore/datastore.go (right): https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor... service/datastore/datastore.go:385: n := runtime.Callers(0, pc) On 2016/01/15 04:32:53, iannucci wrote: > try > > for _, elem := range pc[:runtime.Callers(0, pc)] { > } Done. https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor... service/datastore/datastore.go:406: path, err := getCallingTestFilePath(100) On 2016/01/15 04:32:53, iannucci wrote: > document the 100 element limit in the docstring above > > "If you have more than 100 stack frames between here and your test file, you're > doing it wrong" > > :) Yep :) Done.
Description was changed from ========== GitHub #8: Seed indexes from index.yml - Add `ParseIndexYAML` and `FindAndParseIndexYAML` functions to service/datastore - Add YAML marshalling and unmarshalling for IndexDefinition and IndexColumn - Add tests for the changes - Imports: additional package: gopkg.in/yaml.v2 R=dnj@chromium.org, estaab@chromium.org, iannucci@chromium.org, vadimsh@chromium.org BUG= ========== to ========== GitHub #8: Seed indexes from index.yml - Add `ParseIndexYAML` and `FindAndParseIndexYAML` functions to service/datastore - Add YAML marshalling and unmarshalling for IndexDefinition and IndexColumn - Add tests for the changes - Imports: additional package: gopkg.in/yaml.v2 https://github.com/luci/gae/issues/8 Closes #8 R=dnj@chromium.org, estaab@chromium.org, iannucci@chromium.org, vadimsh@chromium.org BUG= ==========
A thought just occurred to me: we often structure apps like:
frontend/
index.yml
service/
some_test.go
Could we make the find function take a relative path? Then some_test.go
could call FindAndParseIndexYAML("../frontend").
On Thu, Jan 14, 2016, 22:17 <nishanths@utexas.edu> wrote:
>
>
>
https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor...
> File service/datastore/datastore.go (right):
>
>
>
https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor...
> service/datastore/datastore.go:385
>
<https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor...>:
> n := runtime.Callers(0, pc)
> On 2016/01/15 04:32:53, iannucci wrote:
> > try
>
> > for _, elem := range pc[:runtime.Callers(0, pc)] {
> > }
>
> Done.
>
>
>
https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor...
> service/datastore/datastore.go:406
>
<https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor...>:
> path, err :=
> getCallingTestFilePath(100)
> On 2016/01/15 04:32:53, iannucci wrote:
> > document the 100 element limit in the docstring above
>
> > "If you have more than 100 stack frames between here and your test
> file, you're
> > doing it wrong"
>
> > :)
>
> Yep :) Done.
>
> https://chromiumcodereview.appspot.com/1574353004/
>
--
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
Oh and could you change the CL message to include the updated function names (there is an edit link in the top right corner), and instead of Github #8, put Fixes #8 to make github auto-close the issue :) On Fri, Jan 15, 2016, 09:23 Robert Iannucci <iannucci@chromium.org> wrote: > A thought just occurred to me: we often structure apps like: > > frontend/ > index.yml > service/ > some_test.go > > Could we make the find function take a relative path? Then some_test.go > could call FindAndParseIndexYAML("../frontend"). > > On Thu, Jan 14, 2016, 22:17 <nishanths@utexas.edu> wrote: > >> >> >> https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor... >> File service/datastore/datastore.go (right): >> >> >> https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor... >> service/datastore/datastore.go:385 >> <https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor...>: >> n := runtime.Callers(0, pc) >> On 2016/01/15 04:32:53, iannucci wrote: >> > try >> >> > for _, elem := range pc[:runtime.Callers(0, pc)] { >> > } >> >> Done. >> >> >> https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor... >> service/datastore/datastore.go:406 >> <https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor...>: >> path, err := >> getCallingTestFilePath(100) >> On 2016/01/15 04:32:53, iannucci wrote: >> > document the 100 element limit in the docstring above >> >> > "If you have more than 100 stack frames between here and your test >> file, you're >> > doing it wrong" >> >> > :) >> >> Yep :) Done. >> >> https://chromiumcodereview.appspot.com/1574353004/ >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Definitely a good idea on the path parameter for `FindAndParseIndexYAML`. Should I make it an optional parameter? When the path argument is not specified, the function would search up the tree from the test file (like it does now). When the path is specified, it would use the specified path as the base directory to begin the search at. Planning to make it optional using the variable args ... operator. Will also check to make sure that the length of the variables args <= 1. Thoughts? On Fri, Jan 15, 2016 at 11:25 AM, Robert Iannucci <iannucci@chromium.org> wrote: > Oh and could you change the CL message to include the updated function > names (there is an edit link in the top right corner), and instead of > Github #8, put Fixes #8 to make github auto-close the issue :) > > On Fri, Jan 15, 2016, 09:23 Robert Iannucci <iannucci@chromium.org> wrote: > >> A thought just occurred to me: we often structure apps like: >> >> frontend/ >> index.yml >> service/ >> some_test.go >> >> Could we make the find function take a relative path? Then some_test.go >> could call FindAndParseIndexYAML("../frontend"). >> >> On Thu, Jan 14, 2016, 22:17 <nishanths@utexas.edu> wrote: >> >>> >>> >>> https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor... >>> File service/datastore/datastore.go (right): >>> >>> >>> https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor... >>> service/datastore/datastore.go:385 >>> <https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor...>: >>> n := runtime.Callers(0, pc) >>> On 2016/01/15 04:32:53, iannucci wrote: >>> > try >>> >>> > for _, elem := range pc[:runtime.Callers(0, pc)] { >>> > } >>> >>> Done. >>> >>> >>> https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor... >>> service/datastore/datastore.go:406 >>> <https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor...>: >>> path, err := >>> getCallingTestFilePath(100) >>> On 2016/01/15 04:32:53, iannucci wrote: >>> > document the 100 element limit in the docstring above >>> >>> > "If you have more than 100 stack frames between here and your test >>> file, you're >>> > doing it wrong" >>> >>> > :) >>> >>> Yep :) Done. >>> >>> https://chromiumcodereview.appspot.com/1574353004/ >>> >> -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Just make an empty string do what it does now (e.g. "" == ".") On Fri, Jan 15, 2016, 09:37 Nishanth Shanmugham <nishanths@utexas.edu> wrote: > Definitely a good idea on the path parameter for `FindAndParseIndexYAML`. > > Should I make it an optional parameter? When the path argument is not > specified, the function would search up the tree from the test file (like > it does now). When the path is specified, it would use the specified path > as the base directory to begin the search at. > > Planning to make it optional using the variable args ... operator. Will > also check to make sure that the length of the variables args <= 1. > Thoughts? > > On Fri, Jan 15, 2016 at 11:25 AM, Robert Iannucci <iannucci@chromium.org> > wrote: > >> Oh and could you change the CL message to include the updated function >> names (there is an edit link in the top right corner), and instead of >> Github #8, put Fixes #8 to make github auto-close the issue :) >> >> On Fri, Jan 15, 2016, 09:23 Robert Iannucci <iannucci@chromium.org> >> wrote: >> >>> A thought just occurred to me: we often structure apps like: >>> >>> frontend/ >>> index.yml >>> service/ >>> some_test.go >>> >>> Could we make the find function take a relative path? Then some_test.go >>> could call FindAndParseIndexYAML("../frontend"). >>> >>> On Thu, Jan 14, 2016, 22:17 <nishanths@utexas.edu> wrote: >>> >>>> >>>> >>>> https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor... >>>> File service/datastore/datastore.go (right): >>>> >>>> >>>> https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor... >>>> service/datastore/datastore.go:385 >>>> <https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor...>: >>>> n := runtime.Callers(0, pc) >>>> On 2016/01/15 04:32:53, iannucci wrote: >>>> > try >>>> >>>> > for _, elem := range pc[:runtime.Callers(0, pc)] { >>>> > } >>>> >>>> Done. >>>> >>>> >>>> https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor... >>>> service/datastore/datastore.go:406 >>>> <https://chromiumcodereview.appspot.com/1574353004/diff/60001/service/datastor...>: >>>> path, err := >>>> getCallingTestFilePath(100) >>>> On 2016/01/15 04:32:53, iannucci wrote: >>>> > document the 100 element limit in the docstring above >>>> >>>> > "If you have more than 100 stack frames between here and your test >>>> file, you're >>>> > doing it wrong" >>>> >>>> > :) >>>> >>>> Yep :) Done. >>>> >>>> https://chromiumcodereview.appspot.com/1574353004/ >>>> >>> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== GitHub #8: Seed indexes from index.yml - Add `ParseIndexYAML` and `FindAndParseIndexYAML` functions to service/datastore - Add YAML marshalling and unmarshalling for IndexDefinition and IndexColumn - Add tests for the changes - Imports: additional package: gopkg.in/yaml.v2 https://github.com/luci/gae/issues/8 Closes #8 R=dnj@chromium.org, estaab@chromium.org, iannucci@chromium.org, vadimsh@chromium.org BUG= ========== to ========== Fixes #8: Seed indexes from index.yml - Add `ParseIndexYAML` and `FindAndParseIndexYAML` functions to service/datastore - Add YAML marshalling and unmarshalling for IndexDefinition and IndexColumn - Add tests for the changes - Imports: additional package: gopkg.in/yaml.v2 https://github.com/luci/gae/issues/8 Closes #8 R=dnj@chromium.org, estaab@chromium.org, iannucci@chromium.org, vadimsh@chromium.org BUG= ==========
On 2016/01/15 17:56:03, iannucci wrote: > Just make an empty string do what it does now (e.g. "" == ".") > Hey nishanths, just wondering if you got a chance to poke at this? If you don't have time to finish up the patch I can take it over if you like. R
On 2016/01/29 21:55:21, iannucci wrote: > On 2016/01/15 17:56:03, iannucci wrote: > > Just make an empty string do what it does now (e.g. "" == ".") > > > > Hey nishanths, just wondering if you got a chance to poke at this? If you don't > have time to finish up the patch I can take it over if you like. > > R Hey! I made the commits addressing this is Patch Set 6, but missed to leave a comment that it was done. Apologies that it seemed abandoned.
ah sorry my bad! The review system doesn't send emails on new patchset uploads (it's a little bit arcane). lgtm! Thanks :)
The CQ bit was checked by iannucci@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1574353004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1574353004/100001
Message was sent while issue was closed.
Description was changed from ========== Fixes #8: Seed indexes from index.yml - Add `ParseIndexYAML` and `FindAndParseIndexYAML` functions to service/datastore - Add YAML marshalling and unmarshalling for IndexDefinition and IndexColumn - Add tests for the changes - Imports: additional package: gopkg.in/yaml.v2 https://github.com/luci/gae/issues/8 Closes #8 R=dnj@chromium.org, estaab@chromium.org, iannucci@chromium.org, vadimsh@chromium.org BUG= ========== to ========== Fixes #8: Seed indexes from index.yml - Add `ParseIndexYAML` and `FindAndParseIndexYAML` functions to service/datastore - Add YAML marshalling and unmarshalling for IndexDefinition and IndexColumn - Add tests for the changes - Imports: additional package: gopkg.in/yaml.v2 https://github.com/luci/gae/issues/8 Closes #8 R=dnj@chromium.org, estaab@chromium.org, iannucci@chromium.org, vadimsh@chromium.org BUG= Committed: https://github.com/luci/gae/commit/1fb004af6ec3634b29cd97406e17d4bb0a7d2236 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://github.com/luci/gae/commit/1fb004af6ec3634b29cd97406e17d4bb0a7d2236 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
