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

[JSON] getBVal -> getBool; getFNum -> getFloat; getNum -> getBiggestInt; add getInt #6504

Merged
merged 2 commits into from Oct 14, 2017
Merged

Conversation

Yardanico
Copy link
Collaborator

@Yardanico Yardanico commented Oct 13, 2017

Changed procedure names, so they make more sense now.
Old procedure names are now deprecated.
Also removed bountysource, since it lives in the website repo.

@Yardanico Yardanico changed the title [JSON] getBVal -> getBool; getFNum -> getFloat [JSON] getBVal -> getBool; getFNum -> getFloat; getNum -> getBiggestInt; add getInt Oct 14, 2017
@@ -47,7 +47,7 @@ proc createLoadProc(typeName: NimIdent, identDefs: seq[NimNode]): NimNode =
`cfgIdent`.`fieldNameIdent` = `objIdent`[`fieldName`].getStr
of "int":
body.add quote do:
`cfgIdent`.`fieldNameIdent` = `objIdent`[`fieldName`].getNum().int
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't change this. We need to make sure that whatever is in the book continues to work. That's the whole point.

@@ -53,7 +53,7 @@ proc processSupporters(supporters: JsonNode) =
echo("Found ", supporters.elems.len, " named sponsors.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Just remove this file please, it lives in the website repo now.

@dom96 dom96 merged commit f9dc204 into nim-lang:devel Oct 14, 2017
@ghost
Copy link

ghost commented Oct 14, 2017

So how about the fields of JsonNode?? Their named str, num, fnum, bval and are exported. I would ask to revert this.

@Yardanico
Copy link
Collaborator Author

@konqoro generally fields of JsonNode shouldn't be used directly.

@ghost
Copy link

ghost commented Oct 14, 2017

ok @Yardanico how about properly deprecating these procs without removing them? This change breaks a lot of code out there for no reason.

@Yardanico
Copy link
Collaborator Author

@konqoro check the code please, I've deprecated old proc names

@ghost
Copy link

ghost commented Oct 15, 2017

@Yardanico no you didn't code does not compile

@Yardanico
Copy link
Collaborator Author

@konqoro are you sure?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants