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

fix: do not over-fresh type variables #906

Closed
wants to merge 1 commit into from
Closed

fix: do not over-fresh type variables #906

wants to merge 1 commit into from

Conversation

hishamhm
Copy link
Member

Fixes #905.

@hishamhm
Copy link
Member Author

@svermeulen @mtdowling Testing of this PR with your codebases is appreciated!

Copy link

Teal Playground URL: https://906--teal-playground-preview.netlify.app

@mtdowling
Copy link

I'm happy to test this. Is there a way to have Luarocks install a specific branch of tl? You taught me about --dev the other day, but not sure about how to use a branch.

@catwell
Copy link
Contributor

catwell commented Jan 13, 2025

@mtdowling You can add branch = "..." in the source table of the rockspec, but for Teal it is probably easier to clone the repository, checkout the branch you want and run tl directly from there.

@hishamhm
Copy link
Member Author

luarocks build --dev --branch fix-906 tl should work too!

@mtdowling
Copy link

mtdowling commented Jan 13, 2025

I think I tested this and works for my project.

For future reference for my future self, I did this:

  1. Cloned this repo and checked out this branch.
  2. Ran luarocks build in the tl directory.
  3. Verified that the tl.lua file in /opt/homebrew/share/lua/5.4/tl.lua has this change.
  4. Verified that cyan installed via homebrew uses this same path.
  5. Ran cyan to build my project successfully.
  6. Double checked that cyan uses this file by making it error out, and it did... so my assumption was correct that it was used :)

Edit: also, hah, @hishamhm's solution is of course way better, and this change works fine.

@hishamhm
Copy link
Member Author

As I half-suspected, it looks like this is not the right solution; it causes #909. Back to the drawing board.

@hishamhm
Copy link
Member Author

Replacing this with #911 which looks like a more robust fix!

@hishamhm hishamhm closed this Jan 16, 2025
@hishamhm hishamhm deleted the fix-905 branch January 16, 2025 02:44
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.

Regression: Generic error "got T, expected T"
3 participants