-
Notifications
You must be signed in to change notification settings - Fork 229
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
Http support #281
base: master
Are you sure you want to change the base?
Http support #281
Conversation
@@ -2252,12 +2351,23 @@ inline gravity_string_t *gravity_string_new (gravity_vm *vm, char *s, uint32_t l | |||
obj->s = (char *)s; | |||
obj->len = len; | |||
obj->alloc = (alloc) ? alloc : len; | |||
obj->s = mem_alloc(NULL, obj->alloc); |
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.
this was needed to concat, unsure of the consequences
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.
Please check my reviews
src/shared/gravity_hash.c
Outdated
@@ -174,6 +174,21 @@ gravity_hash_t *gravity_hash_create (uint32_t size, gravity_hash_compute_fn comp | |||
return hashtable; | |||
} | |||
|
|||
gravity_hash_t *gravity_hash_create_from (gravity_hash_t *from_hash) { |
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.
gravity_hash_dup
would probably be a better name
assert(0); | ||
} | ||
|
||
char *gravity_map_to_string(gravity_vm *vm, gravity_map_t *map) { |
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.
Why not to reuse convert_map2string
defined in gravity_core.c?
return map_string->s; | ||
} | ||
|
||
gravity_value_t gravity_map_to_value (gravity_vm *vm, gravity_map_t *map) { |
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.
Not sure to understand the use case of this...
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.
@marcobambini Wouldn't you say there's a need for _to_value functions for all types? E.g. if I want to create a map with a key associated to an array, I'd have no way of doing that without doing something like:
gravity_map_t *response = gravity_map_new(vm, 32);
gravity_list_t *headers = gravity_list_new(vm, 32);
gravity_map_insert(vm, response,
gravity_string_to_value("Headers"),
gravity_list_to_value(headers));
If there's a way for already doing something like this, let me know. I'm cleaning up the implementation at the moment, currently trying to get something like this going
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 use the map_to_value method in a similar fashion in src/optionals/gravity_http.c
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.
Oh I guess VALUE_FROM_OBJECT might be helpful in this case 😎
src/shared/gravity_value.c
Outdated
if (s && len) obj->hash = gravity_hash_compute_buffer((const char *)s, len); | ||
|
||
if (vm) gravity_vm_transfer(vm, (gravity_object_t*) obj); | ||
return obj; | ||
} | ||
|
||
inline void gravity_string_concat_cstring (gravity_vm *vm, gravity_string_t *obj, char *cstring) { | ||
uint32_t len = (uint32_t)(strlen(obj->s) + strlen(cstring)); |
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.
obj->len would be much better than strlen(obj->s)
inline void gravity_string_concat_cstring (gravity_vm *vm, gravity_string_t *obj, char *cstring) { | ||
uint32_t len = (uint32_t)(strlen(obj->s) + strlen(cstring)); | ||
uint32_t alloc = MAXNUM(len+1, DEFAULT_MINSTRING_SIZE); | ||
obj->s = mem_realloc(NULL, obj->s, alloc); |
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.
Instead of always perform a men_realloc you should first check if (obj->alloc - obj->len) is big enough
src/shared/gravity_value.c
Outdated
obj->s = mem_realloc(NULL, obj->s, alloc); | ||
obj->len = len; | ||
obj->alloc = alloc; | ||
strcat(obj->s, cstring); |
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.
memcpy would be faster (you already have the right pointer by using (obj->s + obj->len)
Thanks @jamesalbert for your work on Gravity! |
Awesome @marcobambini I'll try and tackle these after work. Np, it's a nice little language, I like the style |
Thanks @jamesalbert and please note that a unit test would be recommended. |
@marcobambini I figured an http client without ssl support is pretty useless. I added openssl support and tried to make it as non-dependent as possible. If the user doesn't have openssl, they can set the optional make parameter Let me know what you think. You can run the examples in docker with |
@jamesalbert I apologize for taking so long in reviewing your pull request but I made a lot of refactoring for optional classes. Can you please remove the files: |
@marcobambini not sure if this is still possible to be revisited. would it be possible to carry this work through with this pr's changes, if it currently isnt in the main release? |
Just want to say I've had a lot of fun with gravity 😄
I created an issue #280 about adding http support. I'm still unsure if this is wanted at this point in the project. With feedback, I'd be happy to refactor this janky pr a bit and implement the rest for an mvp. Or you can tell me my code sucks and to go home.
I've been testing using a basic flask server and the following gravity script: