Skip to content
This repository has been archived by the owner on Mar 22, 2021. It is now read-only.

Add a subset of path manipulation functionality #1

Merged
merged 4 commits into from
Jul 27, 2020

Conversation

gabyx
Copy link
Contributor

@gabyx gabyx commented Jul 20, 2020

pandoc/lua-filters#102 (comment)

As said I am really newbie, had some hard time to figure out how things work.
Thanks for revewing and giving me some advice on how to make it more correct... :-)

I have not yet understood the Lua.liftIO and stuff :-). its quite hard to get into Haskell and at the same time also understand Lua etc ;-)
Thanks for having a look. The tests work and I only added it for unix testing, which is the case on travis and since we anyway only forward to the another library, we leave the proper testing to them.

@gabyx gabyx force-pushed the feature/add-path-manipulations branch from 1eaba89 to c3a1a41 Compare July 20, 2020 20:58
@gabyx gabyx force-pushed the feature/add-path-manipulations branch from c3a1a41 to 546ce86 Compare July 20, 2020 21:00
@@ -33,6 +33,7 @@ library
, exceptions >= 0.8 && < 0.11
, hslua >= 1.0.3 && < 1.2
, temporary >= 1.2 && < 1.4
, filepath >= 1.4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Version might be reviewed here...

Copy link
Member

Choose a reason for hiding this comment

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

Using filepath > 1.4 && < 1.5 will guarantee that there won't be unexpected breaking changes when filepath is updated. Supporting older filepath versions is not necessary, version 1.3 is more than 5 years old.

@tarleb
Copy link
Member

tarleb commented Jul 20, 2020

Thanks for the PR! It might take me a little longer than usual to review, probably not before next week. Thanks for being patient.

@gabyx
Copy link
Contributor Author

gabyx commented Jul 25, 2020 via email

Copy link
Member

@tarleb tarleb left a comment

Choose a reason for hiding this comment

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

Pong 🏓 🙂
Looking good so far. We'll also need to add documentation for all new functions to the README. Those docs will then be copied to the lua-filters manual.

src/Foreign/Lua/Module/System.hs Show resolved Hide resolved
take_filename fp = return $ Fp.takeFileName fp

-- | See @System.FilePath.takeExtensions
take_extensions :: FilePath -> Lua String
Copy link
Member

Choose a reason for hiding this comment

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

Fp.takeExtension has type FilePath -> FilePath, so a type of FilePath -> Lua FilePath would seem more appropriate here (though those are equivalent).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How did you see, that do you have code completion? I am in vs code.
When I look at the docs its: String
https://hackage.haskell.org/package/filepath-1.4.2.1/docs/System-FilePath-Posix.html#v:takeExtensions

Copy link
Member

Choose a reason for hiding this comment

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

You are right, my bad. I was looking at takeExtensions.

src/Foreign/Lua/Module/System.hs Outdated Show resolved Hide resolved
src/Foreign/Lua/Module/System.hs Outdated Show resolved Hide resolved
@@ -33,6 +33,7 @@ library
, exceptions >= 0.8 && < 0.11
, hslua >= 1.0.3 && < 1.2
, temporary >= 1.2 && < 1.4
, filepath >= 1.4
Copy link
Member

Choose a reason for hiding this comment

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

Using filepath > 1.4 && < 1.5 will guarantee that there won't be unexpected breaking changes when filepath is updated. Supporting older filepath versions is not necessary, version 1.3 is more than 5 years old.

@gabyx
Copy link
Contributor Author

gabyx commented Jul 26, 2020

Added the changes. Updated the readme, also fixed linting errors in markdown (-> more commonmark alike -> dont mix heading styles etc...)

@gabyx
Copy link
Contributor Author

gabyx commented Jul 26, 2020

Thanks for merging.

@gabyx
Copy link
Contributor Author

gabyx commented Jul 26, 2020

Do you rather want version 0.3.0 (needs deps version changes in pandoc because it includes till < 3.0. So I though because its only adding new stuff which does not change anything we just bump to 0.2.2 (?)

@gabyx gabyx force-pushed the feature/add-path-manipulations branch from 527ce5d to f5a33e6 Compare July 26, 2020 18:19
README.md Outdated

- `take_directory (filepath)` wraps [System.FilePath.takeDirectory](https://hackage.haskell.org/package/filepath-1.4.2.1/docs/System-FilePath-Posix.html#v:takeDirectory)
Copy link
Member

Choose a reason for hiding this comment

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

These docs will be targeted at Lua users who may not even be aware of Haskell. There should be enough details for anybody to use the functions. Best to use the other docs as reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add the change later

@tarleb
Copy link
Member

tarleb commented Jul 26, 2020

Version 0.2.2 is fine. I usually try to keep version changes separate, but I don't mind much either.

@gabyx
Copy link
Contributor Author

gabyx commented Jul 27, 2020

Checked in the full lua documentation. Added also some examples from the haskell dok, but for lua.

@tarleb tarleb merged commit c1e1bb4 into hslua:master Jul 27, 2020
@tarleb
Copy link
Member

tarleb commented Jul 27, 2020

Great, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants