Skip to content
This repository was archived by the owner on Mar 8, 2020. It is now read-only.

New ToNoder option to fill missing position elements. Unified parser.go skeleton.#166

Merged
juanjux merged 3 commits into
bblfsh:masterfrom
juanjux:feature/generic_to_sdk
Aug 30, 2017
Merged

New ToNoder option to fill missing position elements. Unified parser.go skeleton.#166
juanjux merged 3 commits into
bblfsh:masterfrom
juanjux:feature/generic_to_sdk

Conversation

@juanjux
Copy link
Copy Markdown
Contributor

@juanjux juanjux commented Aug 29, 2017

Part of src-d/backlog/issues/889

@juanjux juanjux requested a review from abeaumont August 29, 2017 12:21
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 29, 2017

Codecov Report

Merging #166 into master will decrease coverage by 2.84%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #166      +/-   ##
==========================================
- Coverage   40.57%   37.72%   -2.85%     
==========================================
  Files          21       21              
  Lines        2223     2396     +173     
==========================================
+ Hits          902      904       +2     
- Misses       1233     1404     +171     
  Partials       88       88
Impacted Files Coverage Δ
protocol/native/objecttonoder.go 64.38% <ø> (ø) ⬆️
protocol/server.proteus.go 60% <0%> (-40%) ⬇️
protocol/generated.pb.go 27.25% <0%> (-9.37%) ⬇️
protocol/protocol.go 25.8% <0%> (-4.97%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6b77be...ade534d. Read the comment docs.

toNoder := &native.ObjectToNoder{}
parser, err := native.ExecParser(toNoder, opts.NativeBin)
func ParserBuilder(opts driver.ParserOptions) (parser driver.Parser, err error) {
psr, err := native.ExecParser(ToNoder, opts.NativeBin)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need for a different (and harder to understand) psr name, is it? You can just reassign parser if needed.

Copy link
Copy Markdown
Contributor Author

@juanjux juanjux Aug 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to differentiate the normal parser from the transformation parsers that take the first as one of their fields (2 and 3 cases in the switch), but maybe using a single parser var is clearer.

psr, err := native.ExecParser(ToNoder, opts.NativeBin)
if err != nil {
return nil, err
return psr, err
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If using named return values, and parser is used instead of psr, return would be enough here.


switch ToNoder.PositionFill {
case native.None:
parser = psr
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this case if psr is named parser

@juanjux juanjux merged commit 1359c99 into bblfsh:master Aug 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants