Skip to content
This repository has been archived by the owner on May 10, 2019. It is now read-only.

Added Server checkers #75

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

sansyrox
Copy link
Member

@sansyrox sansyrox commented Mar 7, 2019

Base PR of issue : fossasia/susi_linux#464
Added Server Checker functions that check if the local server is active or not

@hongquan
Copy link
Member

hongquan commented Mar 9, 2019

I think the behaviour of "having a background task to check and switch between local server and remote server" is just a specific need of an application that uses this lib.

When you are making a lib, you have to make it agnostic to application. If this lib can only be used for one application, better baking it into application.

There are other issues:

  • Server URL is hardcoded. If an application is deployed to a machine where port 4000 is already occupied by some other service, they have to make local SUSI server running on different port. But your lib is hard-coded with port 4000, how can they use it?

I refuse to merge this PR.

@sansyrox
Copy link
Member Author

@hongquan , I don't understand. What do you mean by "lib" ?

@hongquan
Copy link
Member

"lib" = "library", I mean this "susi_api_wrapper". You moved it out of "susi_linux" to act as a library.

@sansyrox
Copy link
Member Author

Must merge for this PR to work .

@sansyrox
Copy link
Member Author

@hongquan @Orbiter please have a look again.

@hongquan
Copy link
Member

I will leave @Orbiter to decide to merge this or not. My opinion is to refuse this, because I don't agree to put a background task into a library. It should be done in application (susi_linux), not library (susi_api_wrapper).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants