-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Rename json::List to json::Array #19114
Conversation
/// Returns true if the Json value is a List. Returns false otherwise. | ||
pub fn is_list<'a>(&'a self) -> bool { | ||
self.as_list().is_some() | ||
/// Returns true if the Json value is a Array. Returns false otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"an Array"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
English is difficult
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in frewsxcv@ef5acff
Object(JsonObject), | ||
Null, | ||
} | ||
|
||
pub type JsonList = Vec<Json>; | ||
pub type JsonArray = Vec<Json>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Json
prefixes of these aliases are unneeded since we have a snapshot, the re-exports of Json
variants won't conflict in the type namespace (as the non-re-exported variants would, previously).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel as though that is a separate (but valid) task unrelated to mine and would be better off in a separate pull request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we can leave that change for a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eddyb I unprefixed the type aliases and it appears they are conflicting with the variants. Am I doing something wrong here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to be a bit careful in how you refer to the typedef:
pub enum Json {
Array(json::Array),
...
}
pub type Array = Vec<Json>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we're planning on leaving them around for 1.0, but I am not the API czar. cc @aturon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened a PR #19224
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of any off the top of my head.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least for this JSON module, I can't think of any reason to keep the re-exports
Thanks, and welcome. :) |
Addressing the issues brought up in [this thread](rust-lang#19114 (comment)) This pull request: * Unpublicizes reexports * Renames type aliases: * `json::JsonArray` ☞ `json::Array` * `json::JsonObject` ☞ `json::Object`
Fixes #19010