search

R1j1t / contextualSpellCheck

✔️Contextual word checker for better suggestions
MIT License
405 stars 56 forks source link

Unnecessary dependency on flask #47

Closed kshitij12345 closed 3 years ago

kshitij12345 commented 3 years ago

From what I see, flask is only used in the example file modelAPI.py in RESTAPI.

The core code is not at all dependent on flask, however it is a required dependency https://github.com/R1j1t/contextualSpellCheck/blob/f8cbeb8a7d5dc085f9f8cc5d27d390848d2df274/requirements.txt#L1

I think it would be better to just add note in modelAPI.py to install flask and remove it from requirements.txt.

Thanks for the interesting project.

R1j1t commented 3 years ago

That is a valid point. My reasoning for not removing Flask was, this repository has a dependency on it. Core functionality dependency is better captured in setup.py below:

https://github.com/R1j1t/contextualSpellCheck/blob/f8cbeb8a7d5dc085f9f8cc5d27d390848d2df274/setup.py#L31

I would not mind having a requirement.txt in RESTAPI folder. But I was not sure so I just left it. I will check on the best practice and then maybe change it.

And, I am happy you liked the project! Please feel free to contribute!

R1j1t commented 3 years ago

I tried to find any relevant PEP to identify what is the recommended way of using requirement.txt but was not able to find anything specific.

PEP 518 talks about minimum build requirement in setup.py (which this follows). I think the discussion is valid and I will be happy to create requirement.txt in RESTAPI/examples. My decision is biased based on a similar experience I had when one of the optional dependencies choked the CI and I had to comment that package. @kshitij12345 do you want to pickup this change?

Other similar PEP but not much relevant: PEP 508 PEP 650