Skip to content

Conversation

ivan-tymoshenko
Copy link
Member

@ivan-tymoshenko ivan-tymoshenko commented May 28, 2022

Checklist

There is possible to have a function names collision. I suggest not overcomplicating things and using only generated function names.

@ivan-tymoshenko ivan-tymoshenko changed the title Fix generated function names collision fix: generated function names collision May 28, 2022
@ivan-tymoshenko ivan-tymoshenko force-pushed the fix-function-name-collision branch from 76ae9f9 to 330acec Compare May 28, 2022 17:25
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

The reason we use real names for functions is to ease debugging. Maybe just add an integer at the end of name?

@ivan-tymoshenko
Copy link
Member Author

It will not solve the problem. I can create a field with the same name as another field name + counter.

@ivan-tymoshenko
Copy link
Member Author

I don't know if it's really useful. If you mean useful in the development process, it's pretty clear what this is a function, because you see inlined arguments. If you talking about an error call stack, I would say we should always throw an explainable error and never use a runtime generated call stack to find the reason for the error.

But maybe I don't see some useful cases.

@mcollina
Copy link
Member

I developed this module by printing out the generated source code, identify which function was causing a problem, and then change the generator. Anonymous functions would prevent this use case, or make it significantly harder.

You can achieve the same by adding a comment in the function itself.

@ivan-tymoshenko
Copy link
Member Author

ivan-tymoshenko commented May 29, 2022

@mcollina What do you think about commenting object properties and their types?

    // {"b":{"type":"string"},"t":{"type":"object"}}
    function anonymous2 (input) {}

@mcollina
Copy link
Member

What I need is the "property chain" that lead to that function., what in the original code was name + key + subkey. This always indicated the path to a given object.

@ivan-tymoshenko
Copy link
Member Author

I will think how to do it clearly. I don't want to pass additional function argument through bunch of core functions to use it in comments in the generated code.

@ivan-tymoshenko ivan-tymoshenko force-pushed the fix-function-name-collision branch from 330acec to c828957 Compare May 29, 2022 16:40
@ivan-tymoshenko
Copy link
Member Author

@mcollina I rewrote it. It adds a comment with function name. You can merge it, but I still think that it's overcomplicate the code.

@ivan-tymoshenko ivan-tymoshenko force-pushed the fix-function-name-collision branch from c828957 to c7c5ae1 Compare May 29, 2022 16:50
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ivan-tymoshenko ivan-tymoshenko force-pushed the fix-function-name-collision branch from c7c5ae1 to 805ca68 Compare May 29, 2022 18:54
@mcollina mcollina merged commit 074a120 into fastify:master May 29, 2022
@ivan-tymoshenko ivan-tymoshenko deleted the fix-function-name-collision branch May 31, 2022 18:24
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.

2 participants