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

feature-#189 #190

Merged
merged 9 commits into from
Aug 16, 2021
Merged

feature-#189 #190

merged 9 commits into from
Aug 16, 2021

Conversation

lcsdavid
Copy link
Contributor

@lcsdavid lcsdavid commented Aug 6, 2021

feature-#189

  • Added object constructor and assignment for json::wvalue and unit tests.
    • json::wvalue::wvalue(std::initializer_list<std::pair<std::string const, json::wvalue>>
    • json::wvalue::wvalue(std::[unordered_]map<std::string, json::wvalue> const&)
    • json::wvalue::wvalue(std::[unordered_]map<std::string, json::wvalue>&&)
    • json::wvalue::operator=(std::initializer_list<std::pair<std::string const, json::wvalue>>)
    • json::wvalue::operator=(std::[unordered_]map<std::string, json::wvalue> const&)
    • json::wvalue::operator=(std::[unordered_]map<std::string, json::wvalue>&&)

@The-EDev The-EDev linked an issue Aug 6, 2021 that may be closed by this pull request
@The-EDev
Copy link
Member

The-EDev commented Aug 6, 2021

Thanks a lot, you went above and beyond doing the unit tests, I'll just look into a couple possible changes, make an example, and this will be merged :)

Thanks again for your work.

@The-EDev
Copy link
Member

The-EDev commented Aug 8, 2021

I've looked further into the code and noticed the fact that you can't exactly do something like

crow::json::wvalue value ({
{"x", 1},
{"y", "two"}
});

which to some extent makes it a comparison between

crow::json::wvalue result;
result["userid"] = userid;
result["email"] = email;
result["firstname"] = firstname;
result["lastname"] = lastname;
return result;

and

crow::json::wvalue result, userid, email, firstname, lastname;
username = 5;
email = "email@example.com";
firstname = "first_name";
lastname = "last_name";
result = {
  {"userid", userid},
  {"email", email},
  {"firstname", firstname},
  {"lastname", lastname}
};

I've seen an example of a multi-type list here and here. Another way is to just have the constructor somehow parse whatever type into json::wvalue and create the map using that. If we are to implement this feature (which I absolutely want), it would be best to implement it in a way where you could have something like the first example.

@lcsdavid
Copy link
Contributor Author

lcsdavid commented Aug 9, 2021

I'll had some exemples to enlighten and show when you can and can't use implicit conversions with those constructors and assignments.
I personnaly advise against using those 'multi-types associative containers' as they add too much boiler plate, and do not solve really this question, and creates hardly understandable template error messages.

@lcsdavid
Copy link
Contributor Author

lcsdavid commented Aug 9, 2021

I actually resolved the "issue", there were no implicit constructor for each supported types in json::wvalue definition.
Without those implicit conversion cannot be performed and std::initializer_list will not compile as expected.
I'll commit soon, feel free to add any remarks, I'll do changes.

Lucas David added 2 commits August 9, 2021 14:32
…ests.

  + json::wvalue::wvalue(std::initializer_list<std::pair<std::string const, json::wvalue>>
  + json::wvalue::wvalue(std::[unordered_]map<std::string, json::wvalue> const&)
  + json::wvalue::wvalue(std::[unordered_]map<std::string, json::wvalue>&&)
  + json::wvalue::operator=(std::initializer_list<std::pair<std::string const, json::wvalue>>)
  + json::wvalue::operator=(std::[unordered_]map<std::string, json::wvalue> const&)
  + json::wvalue::operator=(std::[unordered_]map<std::string, json::wvalue>&&)
  + added corresponding tests.
+ Added json-map examples.
@The-EDev
Copy link
Member

Looks alright to me. I don't have as much experience as I would like though so @mrozigor do you see anything that might be problematic?

@The-EDev The-EDev requested a review from mrozigor August 11, 2021 14:05
include/crow/json.h Outdated Show resolved Hide resolved
include/crow/json.h Outdated Show resolved Hide resolved
examples/example_json_map.cpp Outdated Show resolved Hide resolved
include/crow/json.h Show resolved Hide resolved
include/crow/json.h Outdated Show resolved Hide resolved
tests/unittest.cpp Outdated Show resolved Hide resolved
@mrozigor
Copy link
Member

Overall it looks like a good idea to me ;) I've added a couple of comments.

Copy link
Contributor Author

@lcsdavid lcsdavid left a comment

Choose a reason for hiding this comment

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

Changes done.

@The-EDev
Copy link
Member

Thanks for your work @lcsdavid, I have to add this to the JSON Guide and make a couple changes here and there, but for now I'll merge it into master. Thanks again for doing a great job!

@The-EDev The-EDev merged commit 526fd75 into CrowCpp:master Aug 16, 2021
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.

crow::json::wvalue lacks an 'object' initialization and assignment.
3 participants