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 powershell script equivalents of test scripts and add windows to the test matrix for the test pipeline #42

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions .github/workflows/test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,17 @@ on: [ push ]

jobs:
test:
runs-on: ubuntu-latest
strategy:
matrix:
neovim-version: [ "v0.9.0", "v0.9.1", "stable", "nightly" ]

os: [ windows-latest, ubuntu-latest ]
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v3
- uses: MunifTanjim/setup-neovim-action@v1
- uses: actions/checkout@v4
- uses: rhysd/action-setup-vim@v1
Copy link
Owner

Choose a reason for hiding this comment

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

Does this one support Windows when the other didn't or something like that?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. This supports windows when the previous one does not

with:
tag: ${{ matrix.neovim-version }}
neovim: true
version: ${{ matrix.neovim-version }}
- name: Install Neovim plugins
run: |
./script/setup-test-deps
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
/.clj-kondo/
/.test-config/
/.test-state/
3 changes: 2 additions & 1 deletion fnl/nfnl/compile.fnl
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
(local header-marker "[nfnl]")

(fn with-header [file src]
(.. "-- " header-marker " Compiled from " file " by https://github.com/Olical/nfnl, do not edit.\n" src))
(let [file (fs.standardize-path file)] ;; Normalize the path for Windows
(.. "-- " header-marker " Compiled from " file " by https://github.com/Olical/nfnl, do not edit.\n" src)))

(fn safe-target? [path]
"Reads the given file and checks if it contains our header marker on the
Expand Down
9 changes: 8 additions & 1 deletion fnl/nfnl/core.fnl
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,12 @@
(tset t k nil)))
nil)

(fn dbg [x]
"Prints each of the arguments using vim.inspect and returns them.
Great for debugging some confusing code without changing the behavior"
(print (vim.inspect x))
x)
Copy link
Owner

Choose a reason for hiding this comment

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

Good idea, quite a common function to write repeatedly.


{: rand
: nil?
: number?
Expand Down Expand Up @@ -458,4 +464,5 @@
: constantly
: distinct
: sort
: clear-table!}
: clear-table!
: dbg}
13 changes: 12 additions & 1 deletion fnl/nfnl/fs.fnl
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,15 @@
(replace-extension "lua")
(replace-dirs "fnl" "lua")))

(fn standardize-path [path]
"Replaces all non standard path separators with the standard forward slash"
(str.replace path "\\" "/"))

(fn correct-separators [path]
"Replaces all path separators with the ones appropriate for this system"
(str.replace path "\\" (path-sep))
(str.replace path "/" (path-sep)))
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure of the right answer yet, but something about how these two functions work makes me think there will be issues 🤔 also I didn't think string.gsub mutated the original string? So only the second str.replace actually does things here right?

My thinking is along the lines of "you can have a \ inside a unix path", and I guess you can have a / in a Windows path. So we want to make sure those stay intact when messing with paths. The Conjure code for this probably falls into the same trap, I'll have to check.

Copy link
Author

Choose a reason for hiding this comment

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

Ugh. This is just my not being so familiar with fennel. Sounds like its better to use vim.fs.normalize than my changes


{: basename
: filename
: file-name-root
Expand All @@ -108,4 +117,6 @@
: join-path
: read-first-line
: replace-dirs
: fnl-path->lua-path}
: fnl-path->lua-path
: standardize-path
: correct-separators}
7 changes: 6 additions & 1 deletion fnl/nfnl/string.fnl
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,15 @@
(= suffix (string.sub s (- s-len suffix-len -1)))
false)))

(fn replace [s from to]
"Replace all occurrences of from with to in the string."
(string.gsub s from to))
Copy link
Owner

Choose a reason for hiding this comment

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

So gsub is actually doing pattern based replacement, not whole string replacement. This will cause issues if someone uses Lua pattern syntax without realising.

I don't think this needs to be wrapped in a function here and I'm totally happy for the rest of the code to call into string.gsub directly, no need for the indirection imo.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah duh. My bad


{: join
: split
: blank?
: triml
: trimr
: trim
: ends-with?}
: ends-with?
: replace}
18 changes: 17 additions & 1 deletion fnl/spec/nfnl/config_spec.fnl
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
(local {: describe : it} (require :plenary.busted))
(local core (autoload :nfnl.core))
(local assert (require :luassert.assert))
(local config (require :nfnl.config))
(local fs (require :nfnl.fs))
Expand Down Expand Up @@ -32,9 +33,24 @@
(assert.is_true (config.config-file-path? ".nfnl.fnl"))
(assert.is_false (config.config-file-path? ".fnl.fnl"))))))

(describe
"find"
(fn []
(it "finds the nearest .nfnl file to the given path"
(fn []
(assert.equals
(fs.join-path (fs.full-path ".") ".nfnl.fnl")
(fs.join-path ".nfnl.fnl" (config.find ".")))))))

(describe
"find-and-load"
(fn []
; (it "can read found path securely"
; (fn []
; (let [config-file-path (config.find ".")
; config-source (vim.secure.read (core.dbg (fs.standardize-path config-file-path)))]
; (assert.equals "{:verbose true}" config-source))))

(it "loads the repo config file"
(fn []
(let [{: cfg : root-dir : config}
Expand All @@ -45,7 +61,7 @@

(it "returns an empty table if a config file isn't found"
(fn []
(assert.are.same {} (config.find-and-load "/some/made/up/dir"))))))
(assert.are.same {} (config.find-and-load (fs.correct-separators "/some/made/up/dir")))))))

(fn sorted [xs]
(table.sort xs)
Expand Down
45 changes: 34 additions & 11 deletions fnl/spec/nfnl/fs_spec.fnl
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@
(local assert (require :luassert.assert))
(local fs (require :nfnl.fs))

(fn windows? [] (= jit.os "Windows"))

(describe
"basename"
(fn []
(it "removes the last segment of a path"
(fn []
(assert.equals "foo" (fs.basename "foo/bar.fnl"))
(assert.equals "foo/bar" (fs.basename "foo/bar/baz.fnl"))
(assert.equals "foo" (fs.basename (fs.join-path ["foo" "bar.fnl"])))
(assert.equals (fs.join-path ["foo" "bar"]) (fs.basename (fs.join-path ["foo" "bar" "baz.fnl"])))
(assert.equals "." (fs.basename "baz.fnl"))))

(it "happily lets nils flow back out"
Expand All @@ -18,9 +20,11 @@
(describe
"path-sep"
(fn []
(it "returns the OS path separator (test assumes Linux)"
(it "returns the OS path separator"
(fn []
(assert.equals "/" (fs.path-sep))))))
(if (windows?)
(assert.equals "\\" (fs.path-sep))
(assert.equals "/" (fs.path-sep)))))))

(describe
"replace-extension"
Expand All @@ -34,22 +38,41 @@
(fn []
(it "splits a path into parts"
(fn []
(assert.are.same ["foo" "bar" "baz"] (fs.split-path "foo/bar/baz"))
(assert.are.same ["" "foo" "bar" "baz"] (fs.split-path "/foo/bar/baz"))))))
(assert.are.same ["foo" "bar" "baz"] (fs.split-path (fs.correct-separators "foo/bar/baz"))z)
(assert.are.same ["" "foo" "bar" "baz"] (fs.split-path (fs.correct-separators "/foo/bar/baz")))))))

(describe
"join-path"
(fn []
(it "joins a path together"
(fn []
(assert.equals "foo/bar/baz" (fs.join-path ["foo" "bar" "baz"]))
(assert.equals "/foo/bar/baz" (fs.join-path ["" "foo" "bar" "baz"]))))))
(assert.equals "foo/bar/baz" (fs.standardize-path (fs.join-path ["foo" "bar" "baz"])))
(assert.equals "/foo/bar/baz" (fs.standardize-path (fs.join-path ["" "foo" "bar" "baz"])))))))

(describe
"replace-dirs"
(fn []
(it "replaces directories in a path that match a string with another string"
(fn []
(assert.equals "foo/lua/bar" (fs.replace-dirs "foo/fnl/bar" "fnl" "lua"))
(assert.equals "/foo/lua/bar" (fs.replace-dirs "/foo/fnl/bar" "fnl" "lua"))
(assert.equals "/foo/nfnl/bar" (fs.replace-dirs "/foo/nfnl/bar" "fnl" "lua"))))))
(assert.equals "foo/lua/bar" (fs.standardize-path (fs.replace-dirs (fs.correct-separators "foo/fnl/bar") "fnl" "lua")))
(assert.equals "/foo/lua/bar" (fs.standardize-path (fs.replace-dirs (fs.correct-separators "/foo/fnl/bar") "fnl" "lua")))
(assert.equals "/foo/nfnl/bar" (fs.standardize-path (fs.replace-dirs "/foo/nfnl/bar" "fnl" "lua")))))))

(describe
"standardize-path"
(fn []
(it "replaces all path separators with forward slash"
(fn []
(assert.equals "foo/bar/baz.fnl" (fs.standardize-path "foo\\bar\\baz.fnl"))
(assert.equals "foo/bar/baz.fnl" (fs.standardize-path "foo/bar/baz.fnl"))))))

(describe
"correct-separators"
(fn []
(it ""
(fn []
(if (windows?)
(do (assert.equals "foo\\bar\\baz.fnl" (fs.correct-separators "foo/bar/baz.fnl"))
(assert.equals "foo\\bar\\baz.fnl" (fs.correct-separators "foo\\bar\\baz.fnl")))
(do (assert.equals "foo/bar/baz.fnl" (fs.correct-separators "foo/bar/baz.fnl"))
(assert.equals "foo/bar/baz.fnl" (fs.correct-separators "foo\\bar\\baz.fnl"))))))))
5 changes: 3 additions & 2 deletions lua/nfnl/compile.lua

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lua/nfnl/config.lua

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 6 additions & 2 deletions lua/nfnl/core.lua

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 9 additions & 2 deletions lua/nfnl/fs.lua

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 5 additions & 2 deletions lua/nfnl/string.lua

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 22 additions & 14 deletions lua/spec/nfnl/config_spec.lua

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading