Telodendria/Telodendria
Telodendria/Telodendria
3
7
Fork 9
Code Issues 8 Pull requests 1 Projects 3 Releases 4 Wiki Activity Actions

Refactor code to comply with #8 #39

Merged
jordan merged 9 commits from lda/telodendria:refactor-6 into master 2023-11-21 14:11:47 +00:00
Conversation 4 Commits 9 Files changed 7 +203 -229
lda commented 2023-11-02 12:43:44 +00:00
Contributor
Copy link

Just one question: is implementing j2s parsers for endpoints that
require one property really useful?

NOTE: Currently untested. I doubt anything is wrong with this code, but I
am not totally confident about it.

Please review the developer certificate of origin:

  1. The contribution was created in whole or in part by me, and I have
    the right to submit it under the open source licenses of the
    Telodendria project; or
  2. The contribution is based upon a previous work that, to the best of
    my knowledge, is covered under an appropriate open source license and
    I have the right under that license to submit that work with
    modifications, whether created in whole or in part by me, under the
    Telodendria project license; or
  3. The contribution was provided directly to me by some other person
    who certified (1), (2), or (3), and I have not modified it.
  4. I understand and agree that this project and the contribution are
    made public and that a record of the contribution—including all
    personal information I submit with it—is maintained indefinitely
    and may be redistributed consistent with this project or the open
    source licenses involved.
  • I have read the Telodendria Project development certificate of
    origin, and I certify that I have permission to submit this patch
    under the conditions specified in it.
Just one question: is implementing j2s parsers for endpoints that require **one** property *really* useful? NOTE: Currently **untested**. I doubt anything is wrong with this code, but I am not totally confident about it. --- Please review the developer certificate of origin: 1. The contribution was created in whole or in part by me, and I have the right to submit it under the open source licenses of the Telodendria project; or 1. The contribution is based upon a previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the Telodendria project license; or 1. The contribution was provided directly to me by some other person who certified (1), (2), or (3), and I have not modified it. 1. I understand and agree that this project and the contribution are made public and that a record of the contribution—including all personal information I submit with it—is maintained indefinitely and may be redistributed consistent with this project or the open source licenses involved. - [x] I have read the Telodendria Project development certificate of origin, and I certify that I have permission to submit this patch under the conditions specified in it.
lda added 3 commits 2023-11-02 12:43:45 +00:00
lda changed title from WIP: Refactor code to comply with #6 to WIP: Refactor code to comply with #8 2023-11-02 12:44:06 +00:00
jordan commented 2023-11-04 17:31:51 +00:00
Owner
Copy link

Just one question: is implementing j2s parsers for endpoints that require one property really useful?

No, probably not. I would not bother with any endpoints that only have fewer than 5 properties. 5 or more probably makes sense to have a schema for because then the code gets longer.

> Just one question: is implementing j2s parsers for endpoints that require one property really useful? No, probably not. I would not bother with any endpoints that only have fewer than 5 properties. 5 or more probably makes sense to have a schema for because then the code gets longer.
jordan commented 2023-11-08 19:06:24 +00:00
Owner
Copy link

This looks like a great start. When I have time, I will test the endpoints modified to make sure they still function according to the specifications.

This looks like a great start. When I have time, I will test the endpoints modified to make sure they still function according to the specifications.
lda added 1 commit 2023-11-09 06:12:10 +00:00
lda added 1 commit 2023-11-15 12:27:38 +00:00
lda added 1 commit 2023-11-15 12:33:43 +00:00
lda added 1 commit 2023-11-15 12:56:06 +00:00
lda added 1 commit 2023-11-18 18:39:13 +00:00 
This seems to leak a bit of memory, but it seems like it's related
to Telodendria/Cytoplasm#17, hence
why I'm not fixing it there.
lda commented 2023-11-18 18:40:24 +00:00
Author
Contributor
Copy link

Just implemented for POST /login, it seems to leak memory(but it's probably Telodendria/Cytoplasm#17), but aside, it seems to work.

Just implemented for POST /login, it seems to leak memory(but it's probably https://git.telodendria.io/Telodendria/Cytoplasm/issues/17), but aside, it seems to work.
jordan reviewed 2023-11-20 13:57:06 +00:00
src/Routes/RouteLogin.c Outdated
@ -71,0 +86,4 @@
userIdentifier.user = NULL;
userIdentifier.type = NULL;
jordan commented 2023-11-20 13:57:06 +00:00
Owner
Copy link

Looking at these lines:

    loginRequest.user = NULL;
    loginRequest.address = NULL;
    loginRequest.token = NULL;
    loginRequest.medium = NULL;
    loginRequest.password = NULL;
    loginRequest.device_id = NULL;
    loginRequest.initial_device_display_name = NULL;
    loginRequest.identifier = NULL;

    userIdentifier.user = NULL;
    userIdentifier.type = NULL;

Maybe we should just do this instead:

memset(&loginRequest, 0, sizeof(LoginRequest));
memset(&userIdentifier, 0, sizeof(LoginRequestUserIdentifier));

That way, if more fields get added to either of these structures, we aren't at risk of accidentally adding undefined behavior if we forget to initialize the fields here.

Thoughts?

Looking at these lines: ```c loginRequest.user = NULL; loginRequest.address = NULL; loginRequest.token = NULL; loginRequest.medium = NULL; loginRequest.password = NULL; loginRequest.device_id = NULL; loginRequest.initial_device_display_name = NULL; loginRequest.identifier = NULL; userIdentifier.user = NULL; userIdentifier.type = NULL; ``` Maybe we should just do this instead: ```c memset(&loginRequest, 0, sizeof(LoginRequest)); memset(&userIdentifier, 0, sizeof(LoginRequestUserIdentifier)); ``` That way, if more fields get added to either of these structures, we aren't at risk of accidentally adding undefined behavior if we forget to initialize the fields here. Thoughts?
lda marked this conversation as resolved
jordan reviewed 2023-11-20 13:58:15 +00:00
src/Routes/RouteLogin.c
@ -43,12 +45,9 @@ ROUTE_IMPL(RouteLogin, path, argp)
jordan commented 2023-11-20 13:58:15 +00:00
Owner
Copy link

This line:

 JsonValue *val;

appears to declare an unused variable. If it isn't needed, can you remove this line?

This line: ```c JsonValue *val; ``` appears to declare an unused variable. If it isn't needed, can you remove this line?
jordan commented 2023-11-20 14:27:05 +00:00
Owner
Copy link

Nevermind, I was on the wrong commit. I forgot to do a git fetch.

Nevermind, I was on the wrong commit. I forgot to do a `git fetch`.
jordan marked this conversation as resolved
jordan commented 2023-11-20 14:32:45 +00:00
Owner
Copy link

I just did some very basic testing and everything appears to work fine. Good work. When you're ready for me to merge this, just remove the WIP: prefix as always.

Thanks for your work on this, it is much appreciated.

I just did some very basic testing and everything appears to work fine. Good work. When you're ready for me to merge this, just remove the `WIP: ` prefix as always. Thanks for your work on this, it is much appreciated.
lda added 1 commit 2023-11-21 05:00:24 +00:00
lda changed title from WIP: Refactor code to comply with #8 to Refactor code to comply with #8 2023-11-21 05:03:50 +00:00
jordan merged commit 3dae19e82d into master 2023-11-21 14:11:47 +00:00
jordan referenced this pull request from a commit 2023-11-21 14:11:49 +00:00
Refactor code to comply with #8 (#39)
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

Something is not working

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
invalid

Something is wrong

  
question

More information is needed

  
wontfix

This won't be fixed

No labels
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Telodendria/Telodendria#39
No description provided.
Delete branch "lda/telodendria:refactor-6"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?