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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Brand new find-python.js #2428

Closed

Conversation

owl-from-hogvarts
Copy link
Contributor

@owl-from-hogvarts owl-from-hogvarts commented Jun 4, 2021

Checklist
  • npm install && npm test passes
  • tests are included
  • in code documentation is changed or added
  • +- (old no, but new yes) commit message follows commit guidelines

Description of change

Note: i preserved old api

So... This changes are the logical continuation of my previos pull request #2254.
The purpose of changes is to modernize find-python.js namely:

  • improve code readability
  • use modern way to work with async stuff (using promises instead of callback hell)
  • improve cooments
  • improve tests (in fact the test for Add support for Unicode characters in paths聽#2254 are cherry-picked from here)
  • modernize logging system
  • make log output more clear
  • colorize output
  • make find-python.js easly extencible (thanks to promises and new architecture)

And as I think i have achived that)

Architecture

assuming that #2254 is merged into master

The common idea of checking haven't changed because old method is working pretty well

The main change in architecture is all about working with async logic. Before find-python.js used callbacks to make deal wiht async stuff. It was not so convinent because of error handling. It was necessary to path error callback down to the call tree. That mean that the programmer need to take eyes on that and even minimal carelessness could lead to error not to be handled. So this approach isn't save enough. We have wonderful Promises which make work with async very convinent as we can no longer care to pass appropriate callback to matching function. More on this topic later.
One more not really big but important change is moving to ES6 syntax with with all its classes etc. This syntax sugar is well playing in terms of code readability.

runChecks()

As before we getting the arrays of checks via getChecks() function. Now runChecks use promises due the reasons discussed above. By the way promises give as more extesible (because code is easier to read and i hope understand 馃槃 ). Checks are running one by one (see note). As previosly we running before (also called pretask. For any check you can specify special function which will be executed just before check itself. Useful when checks need to be skiped under a certain condition) function on checks which have it (it is optional). Have added default logging procedure for checks. Do not use before function for logging only. If before is specified, then no standard logging procedure will be executed so you need to make logging by your hand.

Note: I think checks can be easly make running simultaneously via Promise.allSeatled(). But this functionality need to be polifilled because node.js v10 does not support it 馃槩.

more on Promises

The main thing from where promises are comming is PythonFinder.prototype.run. It is wrapper around childProcess.execFile. We have a bit of logging stuff here and a huge comment on possible problems. Check it please. I think it need to be discussed. Important thing is that find-python-script.py mustn't write anything to stderr as it wil be considered as error.
I'd like to make the module inside more modular. Now we have three or so main check funtions. They do not act i would like: like a small functions with only one resposability which can be easily chaned into conveyor. It whould be really cool change. I am already took some steps in this direction: distinguish and generalize logging and move as many error handling to one single place (more on that later).

Note to discuss: generally we can use utils.promisify in PythonFinder.prototype.run???

The rudiments of the conveyor approach

PythonFinder.prototype.checkCommand and PythonFinder.prototype.checkPyLauncher became really thin by using conveyor approach: chaning operations one by one.

New error handling system

Now all errors are handled individually for each check and in one single place PythonFinder.prototype.catchErrors. So you can just throw error and it will be handled and logged properly (default logging mechanism is implemented). If you need some special logging for you error you can make custom error class which should be the successor of ErrorWithData. Later whould be really cool to move to typescript and use here common interface for logging.

Logging

As mentioned before we have almost aoutomotive logging so no need to make custom spikes. Errors are logged in PythonFinder.prototype.catchErrors either by default logger or by implemented log method of custom error, logging which idicates that the check is processing resides in PythonFinder.prototype.runChecks.
The logger itself is a separate class. I decided to move it away from PythonFinder because logging is not responsability of PythonFinder so for logging stuff nothing to do there. Logger is injected in PythonFinder in constructor so can be easely used in it. In my opinion it whould be greate to make the Logger fully separated identity as now there prefix hardcoded in it.
The Logger also include logic to highlight text for terminal


Edited: Also pls see these PRs: owl-from-hogvarts/pull/13, owl-from-hogvarts/pull/12, owl-from-hogvarts/pull/11 as they contain changes that need to be discussed

I hope i stated all my thoughts)

@rvagg @DeeDeeG @joaocgreis you are welcom to review this)

owl-from-hogvarts and others added 25 commits June 3, 2021 20:14
On Windows 10 i discovered an issue that if there are
non-english letters or symbols in path
for python find-python.js script can't find it.
This bug is couse by encoding issue which
I have (i hope) fixed. At least this bug fix works for me.

Have changed:
       modified:   gyp/pylib/gyp/easy_xml.py
         python 2.7 handle xml_string in wrong way
         becouse of encoding (line 128)
       modified:   gyp/pylib/gyp/input.py
         if "encoding:utf8" on line 240 doesn't marked cause to error
       new file:   lib/find-python-script.py
         i have created this file for convience.
         Script which can handle non-eanglish
         letters and symbols cann't 掳fit one lne
         becouse it is necessary to specify
         instarctions for several versions of python
       modified:   lib/find-python.js
         to make js understand python (line 249)
created file test-find-python-script.py
now this script using promises and es6 classes
!!!API HASN'T CHANGE
add documentation
make better logging (colorized output)
make more convenient error handling (one function that catch all errors)
also remove old test for find-python
new-find-python.js -> find-python.js

test-new-find-python.js -> test-find-python.js
Co-authored-by: Christian Clauss <cclauss@me.com>
swap args and name in find-python. reason in comment

add ".js" to couple imports due to import errors
- test/test-find-python-script.js
  rephrase comment

- test/test-find-python.js
  fix equality expression in testExecFile func, which led
  to wrong function behavior

- lib/find-python.js
  - organize colors for highlighting output in single object
  - change the way errors are passed:
    now there is new class ErrorWithData to
    fit the propose to pass additional data along the error.
    Now any error capable object can be thrown

  - general improvements to comments:
    add additional explanations,
    expand documentation,
    provide thoughts about checkPyLauncher,
    outline potential bug with powershell and quoted string on windows,

  - fix error message in "checkExecPath",
  - add version in "checkExecPath" to silly log and return silly logs
    from previous version of "run" function

  -
remove the use of deprecated tap aliases
add simplest polyfill for `fs.rmSync(path, {recursive: true})`
now here is Logger class with all logging functions
and all errors except regular Error class should implement log method
@lukekarrys
Copy link
Member

Thanks for this work. Unfortunately this file has recently been modernized in #2925 so there are a lot of conflicts here. I'm going to close this but feel free to open any new PRs to refactor, those are always appreciated.

@lukekarrys lukekarrys closed this Nov 2, 2023
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

Successfully merging this pull request may close these issues.

None yet

3 participants