Skip to content
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.

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

Unify JSON conversion code #743

Open
rsmmr opened this issue Jan 18, 2020 · 2 comments
Open

Unify JSON conversion code #743

rsmmr opened this issue Jan 18, 2020 · 2 comments

Comments

@rsmmr
Copy link
Member

@rsmmr rsmmr commented Jan 18, 2020

We have two implementations of turning Vals into JSON: Val.cc and threading/formatters/JSON.cc both have BuildJSON() functions/methods that share a lot of similar code, but produce slightly different output. We should refactor them into a single function that can be controlled with options to produce the desired output style.

@rsmmr rsmmr changed the title Unify JSON code Unify JSON conversion code Jan 18, 2020
@timwoj

This comment has been minimized.

Copy link
Contributor

@timwoj timwoj commented Jan 19, 2020

The issue with merging them was that they take two different value types as arguments. The val in threading::Value isn't exactly the same as the one in Val (which is a BroValUnion).

@rsmmr

This comment has been minimized.

Copy link
Member Author

@rsmmr rsmmr commented Jan 20, 2020

Yeah, true -- still, the code feels just too similar to have two implementations (which also do the same things somewhat differently). However, I guess this mean it's not actually a "good first issue" as it's unclear how to actually merge them.

@rsmmr rsmmr removed the good first issue label Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.