change in validation and set_python_target function #1335
Conversation
| }, | ||
| { |
There was a problem hiding this comment.
These should be on the same line...
| }, { | ||
| .name = nxt_string("factory"), | ||
| .type = NXT_CONF_VLDT_STRING, | ||
| // need to add validator here which specify it needs to be mutually exclusive | ||
| // with callable option. not sure at present how | ||
| }, |
There was a problem hiding this comment.
You may as well put this above prefix so the order matches as above...
| } | ||
|
|
||
| obj = PyDict_GetItemString(PyModule_GetDict(module), callable); | ||
| if (nxt_slow_path(obj == NULL)) { | ||
| nxt_alert(task, "Python failed to get \"%s\" from module \"%s\"", | ||
| callable, module_name); |
There was a problem hiding this comment.
What is the value of callable now?
| if (!factory_found) { | ||
| value = nxt_conf_get_object_member(conf, &callable_str, NULL); | ||
| if (value == NULL) { | ||
| callable = nxt_alloca(12); | ||
| nxt_memcpy(callable, "application", 12); | ||
|
|
||
| } else { | ||
| nxt_conf_get_string(value, &str); | ||
|
|
||
| callable = nxt_alloca(str.length + 1); | ||
| nxt_memcpy(callable, str.start, str.length); | ||
| callable[str.length] = '\0'; | ||
| } | ||
| obj = PyDict_GetItemString(PyModule_GetDict(module), callable); | ||
| } | ||
|
|
There was a problem hiding this comment.
I have a feeling all this logic could be cleaned up and possibly removing the need for the factory_found boolean...
There was a problem hiding this comment.
Thanks for the patch!
However the commit message is missing (a few things)
- A description of what this patch does
- Why it's needed
- Examples of valid config using this new feature
I'm guessing it's in relation to #1106
It could do with a more accurate subject, python: Add 'factory' support or some such...
However this doesn't seem to implement @callahad's suggestion. Some text describing the reasoning wouldn't go amiss...
|
It does match my earlier suggestion of
😆 Great way to get the ball rolling; thank you @gourav-kandoria! Are you up for refining this to match the later proposal (a separate |
|
@callahad @ac000 oh my bad really sorry . @callahad I had these changes earlier in my local. raised pr with those changes. |
|
Great, thank you! Closing in favor of #1336 |
No description provided.