Closed sansyrox closed 5 years ago
Dependant on PR(https://github.com/fossasia/susi_api_wrapper/pull/75)
Only today I look into susi_api_wrapper code and found that it is using global
variable. This practice is discouraged in Python, especially dangerous when it is used in multi-threaded app. Modifying shared state is the source of crash.
So, I don't want to merge this PR. I leave it for other reviewers.
So, there are two possible solutions that I have in my mind.
One is that I use an already existing function in the App but it'll still use global variables. But since the multithreading is being done in the client side and the global variables are being used in the API wrapper , so I believe it should not be a problem.
Convert the complete wrapper into an OOP model and remove the FP part. (Better Practice) . However, it can maybe lead to some bugs and I don't want that to happen it before the FOSSASIA Summit.(But we can convert it after the Summit)
@hongquan , what do you suggest?
The option 1 sounds more reasonable, because it moved "multi-threading" to app, not the lib (API wrapper). "global" variables still exists, but need to be removed some day.
The option 2 sounds weird when you think that FP is worse than OOP, especially in the case of multi-threading. FP is better than OOP in multi-threading programming. Maybe just your typo mistake?
I am unsure what to do here. One point is that it is good to discuss a good coding style, another is to get things running in any way to make FOSSASIA next week a success. I am bringing 14 devices to sell there, so everything should work! As Argument 1 is absolutely valid we should not stop here because of technical debts.
This created a crash and had to be reverted: https://github.com/fossasia/susi_linux/commit/ac22e79e85a52a6be8ac862e1794254552fef3a6
Sorry for that 😓 . Log shows that it crashed because of the dependent PR not being merged. I'll make the fix in a single PR without altering the lib after the summit
This created a crash and had to be reverted:
Fixes #464
Checklist
Test Passing
Short description of what this resolves:
Changes proposed in this pull request: