Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Be more pythonic #9

Open
obo opened this issue Jan 18, 2021 · 6 comments
Open

Be more pythonic #9

obo opened this issue Jan 18, 2021 · 6 comments
Assignees

Comments

@obo
Copy link
Contributor

obo commented Jan 18, 2021

SLTev is not written in a nice python way. Aside from worse readability, it brings needless platform dependence.
For instance making os.system calls like this one:

os.system(cmd)

is:

  • slower (a new process is started)
  • risky (filenames get passed to command line and back)
  • likely platform-dependent
  • very likely also insecure (someone can plug in a different "evaluator.py"

Make SLTev a module which can be either imported or directly called (it should contain the standard test at the end and call its "main" if it is run directly).

If people only import SLTev, they should have access to the evaluation routines, to be directly called from within the python. Avoid external tools as much as possible! (with the exception of mwerSegmenter or GIZA).

@pyRis
Copy link
Collaborator

pyRis commented Jan 18, 2021

A more suitable way will be to use subprocess library. In general, subprocess is almost always preferred over os.system() calls.

@obo
Copy link
Contributor Author

obo commented Jan 18, 2021

The point here is not to achieve any parallelization (for which subprocess would be the solution). The way os.system() is used in current SLTev is just to make a function call. :-//

@obo obo assigned pyRis and mohammad2928 and unassigned eebism Jan 28, 2021
@obo
Copy link
Contributor Author

obo commented Jan 28, 2021

Rishu, could you please test the current code and close this issue if SLTev can be now used both:

  • as module
  • as a script

From the code, it looks good, but I have not succeeded with import SLTev due to the fact that SLTev lives in the subdirectory SLTev-scripts, see also #21.

@obo
Copy link
Contributor Author

obo commented Feb 3, 2021

Matusi, this is also something you might immediately see and close.
If SLTev is easy to use both as a script and as a module directly from python, mark this issue as closed.
Thanks, O.

@mzilinec
Copy link
Contributor

mzilinec commented Mar 8, 2021

@obo I think that it is usable as both script and a module, but still, the title "Be more pythonic" - is not the case yet.
There are still things such as calling python evaluator.py with os.system() - this is really not good.

@obo
Copy link
Contributor Author

obo commented Mar 8, 2021

@mzilinec I fully agree. Please rename the issue (or fix the call if it is very easy -- which I do not really expect).

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

No branches or pull requests

5 participants