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

Add --emit-lua-preserve-columns flag #572

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bjornbm
Copy link
Contributor

@bjornbm bjornbm commented Apr 5, 2023

Here is a draft implementation of the --emit-lua-preserve-columns flag proposed in #567. For your comments.

I took largely the path of least resistance. The part that bothers me the most is the overloading of the output_ext to carry the flag, which is a bit of a hack. But I didn't want to embark on a more intrusive refactoring without bouncing it off you first.

Applies to `--only-check and `--emit-lua` flags.
Caused one test to fail. Test case has been corrected.
@bjornbm
Copy link
Contributor Author

bjornbm commented Apr 5, 2023

Regarding the linting failure:

--- Other checks ---
523:local function f(x     , y     )     
524:    return nil       
ERROR File spec/translator_spec.lua has a line that ends in whitespace

For me this is a false warning, since the whitespace is in a multi-line string and therefore has significance. That being said, the concern in the comments in run-lint is valid. The test case could be split into single-line strings, or I could just tack on `]] .. "\n" .. [[" to the end of the offending lines (is there a shorter incantation that would work?).

@bjornbm
Copy link
Contributor Author

bjornbm commented Apr 5, 2023

I pushed the ugly `]] .. "\n" .. [[" fix.

Another option is or course to remove the function return type annotation in the pallene code and the return nil line, but I would argue that they have some value in terms of testing corner case behavior.

@hugomg
Copy link
Member

hugomg commented Apr 9, 2023

Instead of the ]] .. "\n" .. [[" workaround, could we make the Pallene->Lua translator strip whitespace from the end of the lines? That is, don't add whitespace at the end of the line.

In the driver code, the extra preserve_columns and lua_pc seem awkward... But maybe it's the fault of the driver code, which is currently a bit too clever for its own good and begging for some heavy refactoring. It would be nice if we could come up with a cleaner way to do it (you have my permission to refactor as much as you want). Otherwise, my only worry is that whatever hack we use today won't impede a future refactor. I'd love t oknow what you think; you're a fresh set of eyes looking at the driver.lua file and your opinion might be valuable.

@bjornbm
Copy link
Contributor Author

bjornbm commented Apr 10, 2023

Instead of the ]] .. "\n" .. [[" workaround, could we make the Pallene->Lua translator strip whitespace from the end of the lines? That is, don't add whitespace at the end of the line.

We could. Not sure where/how to do it cleanly… maybe as a second pass only to remove trailing whitespace, also applied to vanilla --emit–lua? That would also strip whitespace not in conjunction with types (that is, in the original file and not just due to replacing types with spaces).

For my original motivation I guess it could make sense to remove for the types, to avoid lua tooling/linters complaining about the trailing whitespace.

But for general removal, I'm less sure… there I guess I would maybe want the linter to complain since the whitespace was inserted by the programmer. 🤔

@bjornbm
Copy link
Contributor Author

bjornbm commented Apr 10, 2023

Will think and comment later about the driver code. My first thought when working on it was that at least "file extension" is the wrong level of abstraction for controlling the driver. Should be at minimum a string enum (which is basically what I used it as, I guess) or probably better a table?

@hugomg
Copy link
Member

hugomg commented Apr 10, 2023

Or we could export a separate function for each task, instead of a do-all "compile" function.

@hugomg
Copy link
Member

hugomg commented Apr 10, 2023

About whitespace, I'm curious which Lua linters complain about trailing whitespace.

@bjornbm
Copy link
Contributor Author

bjornbm commented Apr 10, 2023

About whitespace, I'm curious which Lua linters complain about trailing whitespace.

Luacheck, for one. https://luacheck.readthedocs.io/en/stable/warnings.html#formatting-issues-6xx

@hugomg
Copy link
Member

hugomg commented Apr 10, 2023

I see... In that case I agree that we should not add spaces for type annotations, if those spaces would end up at the end of a line. If we remove all spaces at the end of lines, we will silence useful warnings. If we if don't then we'll introduce spurious warnings.

Watch out for multi-line type annotations.

Watch out for multi-line type annotations.

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.

2 participants