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

More nfnl conversion #601

Merged
merged 8 commits into from
Sep 3, 2024
Merged

Conversation

russtoku
Copy link
Contributor

@russtoku russtoku commented Sep 1, 2024

  • Converted:
    • Common Lisp client
    • remote/stdio2.fnl
    • conjure.editor
    • conjure.dynamic
    • extract/searchpair
    • client/fennel/aniseed
  • Added busted tests:
    • remote/stdio2_spec.fnl
    • client/fennel/aniseed_spec.fnl
  • Touched up Racket client (converted nvim.fn to vim.fn)

@russtoku
Copy link
Contributor Author

russtoku commented Sep 1, 2024

Apologies, I pushed my local commit for extract/searchpair and the fennel/aniseed client too soon. It was supposed to be for the next batch.

Unfortunately, the Circle CI build is failing on the lua/conjure-spec/client_spec.lua test now. It looks like the ordering of the items returned by client.each-loaded-client is not what is expected.

Fail || each-loaded-client runs a function for each loaded client
        /home/circleci/project/lua/conjure-spec/client_spec.lua:129: Expected objects to be the same.
        Passed in:
        (table: 0x7fba792d3bd8) {
         *[1] = '.fnl'
          [2] = '.sql' }
        Expected:
        (table: 0x7fba790faa18) {
         *[1] = '.sql'
          [2] = '.fnl' }

@russtoku
Copy link
Contributor Author

russtoku commented Sep 1, 2024

Not sure if it's the right thing to do but I sorted the sequences being compared in the "each-loaded-client runs a function for each loaded client" test in client_spec.fnl to fix the test.

@Olical
Copy link
Owner

Olical commented Sep 3, 2024

Sorting sounds like the right move! I did think that could be flaky at the time, I should've written a better test in the first place. Great catch! Looking at merging this now and will try to fix #602 which I probably caused.

Copy link
Owner

@Olical Olical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@Olical
Copy link
Owner

Olical commented Sep 3, 2024

Looks great, will merge and continue to use in anger at work to try and find issues at runtime. Although I'm only really testing the Clojure client which could be... bad eventually.

I do plan on eventually merging this to main and deprecating master. I won't change the default for a while until it's been in use with a few clients and users for a while. So I think once we're happy that everything is converted I'll start a push to get people over to the new main branch as a sort of "beta". Eventually it'll be the default, but we'll try to make the transition careful.

@Olical Olical merged commit 5dd64b9 into Olical:nfnl-migration Sep 3, 2024
3 checks passed
@Olical
Copy link
Owner

Olical commented Sep 3, 2024

Found an error when trying to autocomplete which is interesting, I think only post merge? Although maybe it was there before. But don't worry, I'm on it 😄 one of the functions is trying to access *module-name* which isn't defined in the cleaned, non-Aniseed, modules.

@Olical
Copy link
Owner

Olical commented Sep 3, 2024

Ah! Here! The Fennel LSP really helps with spotting these undefined references.
image

I'll get that fixed, no big deal. Just need to make sure any references to magic Aniseed values are removed or replaced in some way during the conversion.

Olical added a commit that referenced this pull request Sep 3, 2024
mohsinhassaan pushed a commit to mohsinhassaan/conjure that referenced this pull request Oct 10, 2024
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