Pebaz / nimporter

Compile Nim Extensions for Python On Import!
MIT License
824 stars 33 forks source link

Fix error message checking current line against current line. #10

Closed philippeitis closed 4 years ago

philippeitis commented 4 years ago

It seems like

            line, col = line_col.split(',')
            line = int(line)

in the NimCompileException initializer, followed by

                line = 0
                for each_line in mod:
                    line += 1

                    if line == line:
                        message += f' -> {each_line}'

                    elif line > line + 2:
                        break

                    elif line > line - 3:
                        message += f' |  {each_line}'   

would have the effect of adding every single line in the module to the error message, since we're testing against line, so I just changed it to test against the line in the initial error message.

Pebaz commented 4 years ago

Can you adjust line 52 to read:

src_line = int(src_line)

Because I'm getting a "Unbound local variable" error for "line".

Also, pull master before you push again because I got a chance to add a GitHub Actions workflow to run tests on each platform.

Other than that, great catch ;)

philippeitis commented 4 years ago

Seems to have failed against other parts of the code, which I did not touch.

Also, is there any reason the tests are linked sequentially? They should be able to run in parallel, and skipping tests seems like it'd cause problems with having to wait 6+ minutes to see if mac-os succeeds (or more, if an earlier operating system fails repeatedly).

It seems like you could add this to the workflow to remove some of the duplicate workflows:

    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        os: [macos-latest, ubuntu-latest, windows-latest]
        python-version: [3.6, 3.7, 3.8]
Pebaz commented 4 years ago

That's fantastic, I appreciate the tips!

I'm new to GitHub Actions so this really helps.

(I was doing the sequential thing for debugging but still had the duplicate jobs).

I'll update this as soon as I can so that the pipeline can succeed.

There were some testing edge-cases for Windows that I ran into.

philippeitis commented 4 years ago

To avoid the fail-fast behaviour and have all tests run even on early failure, I think you could add:

strategy:
     fail-fast = False

source page

Pebaz commented 4 years ago

Sorry about the wait, the GitHub Actions pipeline is finally ready. Can you go ahead and pull master and then push your changes?

Pebaz commented 4 years ago

Wait, nevermind, looks like closing and reopening the pull request launched the tests again. If they pass, I'll merge.