Skip to content

BugFix: Src map type assert fix #370

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
barnjamin merged 8 commits into from
Aug 30, 2022
Merged

BugFix: Src map type assert fix #370

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
efae689
Merge branch 'release/v1.14.1'
algojack Mar 29, 2022
6b6dfc0
Merge branch 'release/v1.15.0'
algobarb Apr 28, 2022
7c7bab4
Merge branch 'release/v1.16.0'
onetechnical Jun 3, 2022
fae5b08
Merge branch 'release/v1.17.0'
egieseke Jun 21, 2022
f65c7dc
Merge branch 'release/v1.18.0'
algojack Jul 27, 2022
7dc2807
Merge branch 'release/v1.19.0'
algobarb Aug 17, 2022
450ac63
fix type assertion on source map version
barnjamin Aug 23, 2022
6fb1dc2
Merge branch 'develop' into src-map-type-assert-fix
barnjamin Aug 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion logic/source_map.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@ func DecodeSourceMap(ism map[string]interface{}) (SourceMap, error) {
sm := SourceMap{}
Copy link
Copy Markdown
Contributor

@winder winder Aug 29, 2022

Choose a reason for hiding this comment

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

Alternative:

	buf:=json.Encode(ism)
	json.Decode(buf,&sm)

Copy link
Copy Markdown
Contributor Author

@barnjamin barnjamin Aug 30, 2022

Choose a reason for hiding this comment

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

Can you help me understand why this is better?

Copy link
Copy Markdown
Contributor

@winder winder Aug 30, 2022

Choose a reason for hiding this comment

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

The only reason I bring it up, is that it's easy to get things wrong with reflection. So if there's a way to avoid using it the code would be easier to reason about and less likely that there is an error. So basically, I'm asking if these are simpler alternatives.

Why are you unmarshaling to a map[string]interface{} instead of a structured object?

What I mean is you could use Unmarshal into a real type instead of map[string]interface{}, that way Unmarshal would handle all of this logic. Looking a bit closer just now, I wonder if this is even better:

func DecodeSourceMap(ism map[string]interface{}) (SourceMap, error) {
  buf := json.Encode(ism)
  var sourcemap SourceMap
  return json.Decode(buf, sourcemap)
}

Copy link
Copy Markdown
Contributor Author

@barnjamin barnjamin Aug 30, 2022

Choose a reason for hiding this comment

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

I can do that to begin with to set the fields specified but I still need to do some post-processing to decode the VLQ values. Is it worth it?

Copy link
Copy Markdown
Contributor

@winder winder Aug 30, 2022

Choose a reason for hiding this comment

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

I think it is, but not enough to block your PR so I'll leave it up to you.

Copy link
Copy Markdown
Contributor Author

@barnjamin barnjamin Aug 30, 2022

Choose a reason for hiding this comment

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

I'm going to merge this and post a follow up with the above since I think this is tripping folks up right now


if v, ok := ism["version"]; ok {
sm.Version = int(v.(float64))
switch t := v.(type) {
case float64:
sm.Version = int(t)
case uint64:
sm.Version = int(t)
}
}

if sm.Version != 3 {
Expand Down