Adding relative method in path interface #5

jpuri · 2018-01-06T14:00:20Z

Hey @kennetpostigo , great project.
I was checking it out. I want to contribute to the project and thus I created this PR for a method.

I want to implement path.relative method. What I understand about the method is:

returns the relative path from -> to
if from and to each resolve to the same path return zero length string
if either of from or to are zero length string use current working directory instead of them

Please suggest if I am missing out on anything.

kennetpostigo · 2018-01-06T16:16:18Z

Hey @jpuri ! that sounds just about right. My strategy so far in implementing path operators is to check out the node.js doc examples add a test case with that example. I then also try out a bunch of examples that I want to use as test cases and copy the result from node.js repl, as the expected output for the tests.

I would add test cases with:

absolute paths
relative paths
unnormalized paths
normalized paths

jpuri · 2018-01-07T14:34:14Z

I had some troubles in running tests with jbuilder with latest version of ocaml 4.06.0 but it works perfect in version 4.02.3.

kennetpostigo · 2018-01-07T15:31:45Z

Interesting, do you think you can describe the issues @jpuri ?

jpuri · 2018-01-07T15:43:33Z

Seems to be trouble in some opam packages, its not able to install all required packages for running the test cases:

Jyotis-Air:reason-node jyotipuri$ opam install alcotest
The following actions will be performed:
  ∗  install topkg    0.9.1                   [required by cmdliner, astring, fmt]
  ∗  install cmdliner 1.0.2                   [required by alcotest]
  ∗  install astring  0.8.3                   [required by alcotest]
  ∗  install fmt      0.8.5                   [required by alcotest]
  ∗  install alcotest 0.8.1
===== ∗  5 =====
Do you want to continue ? [Y/n] y

=-=- Gathering sources =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=  🐫 
[alcotest] Archive in cache
[astring] Archive in cache
[cmdliner] Archive in cache
[fmt] Archive in cache
[topkg] Archive in cache

=-=- Processing actions -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=  🐫 
[ERROR] The compilation of topkg failed at "ocaml pkg/pkg.ml build --pkg-name topkg --dev-pkg false".

#=== ERROR while installing topkg.0.9.1 =======================================#
# opam-version 1.2.2
# os           darwin
# command      ocaml pkg/pkg.ml build --pkg-name topkg --dev-pkg false
# path         /Users/jyotipuri/.opam/4.06.0/build/topkg.0.9.1
# compiler     4.06.0
# exit-code    2
# env-file     /Users/jyotipuri/.opam/4.06.0/build/topkg.0.9.1/topkg-49980-c61e7b.env
# stdout-file  /Users/jyotipuri/.opam/4.06.0/build/topkg.0.9.1/topkg-49980-c61e7b.out
# stderr-file  /Users/jyotipuri/.opam/4.06.0/build/topkg.0.9.1/topkg-49980-c61e7b.err
### stdout ###
# Cannot find file topfind.
### stderr ###
# Unknown directive `require'.



=-=- Error report -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=  🐫 
The following actions were aborted
  ∗  install alcotest 0.8.1
  ∗  install astring  0.8.3
  ∗  install cmdliner 1.0.2
  ∗  install fmt      0.8.5
The following actions failed
  ∗  install topkg 0.9.1
No changes have been performed

But I think its not specific to project reason-node.

kennetpostigo · 2018-01-07T15:47:45Z

@jpuri you shouldn't have to use opam, although you could but I don't recommend it. This is probably my fault for not documenting the commands I use to build, test, etc.

So this is usually how the flow of things would go:

esy install
esy build
esy make test

When using esy, you have to prefix commands with esy in order for it to pick up the local environment/sandbox.

jpuri · 2018-01-07T16:02:27Z

Yep I did not prefixed easy before make test. That seems to have created all this trouble.

kennetpostigo · 2018-01-07T16:20:33Z

It works now?

jpuri · 2018-01-07T17:13:21Z

yep works well now

jpuri · 2018-01-08T18:58:09Z

Still WIP. Going slow as I do not have a lot of time.

kennetpostigo · 2018-01-08T19:31:22Z

@jpuri no worries :) Thank you so much for putting time into it!

jpuri · 2018-01-09T17:24:36Z

Hey @kennetpostigo , can you plz check the changes and share ur feedbacks.

kennetpostigo · 2018-01-09T17:27:12Z

@jpuri yes, definitely! I'll review it today

andreypopp · 2018-01-09T20:42:51Z

Hi! I suspect that macOS build fails because esy doesn't produce any output during build process when tty isn't available (like when run on CI). Sometimes build process can take longer than 10m (I saw this with earlier versions of esy too), this can be because travis is out of capacity for macOS machines, for example. See https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received for a workaround.

Next release (soon) I'll fix esy output to provide some progress indication even if no tty is available.

kennetpostigo · 2018-01-09T23:14:04Z

@andreypopp thank you!

kennetpostigo

Hey @jpuri

I went through the code and ran the tests, compared against node.js ouput for same input and I noticed a difference in the output for your relativeWithDifferentPath test. I think this is because the output after diffing both paths should be relative to the current working directory. Both absolute and relative paths seem to be affected.
The implementation doesn't protect against unnormalized paths, first thing you should do before testing diffing the paths is normalize them, luckily theres already a normalize function in path so this should be a quick fix 😀.
In getRelativePath since sometimes code might be ran from a windows machine, you should change~path2=("../" ++ path2) into ~path2=(".." ++ sep ++ path2).
If possible it would be good to add test cases for the new helpers.

kennetpostigo · 2018-01-10T01:48:07Z

Hey @jpuri , The project has been renamed to lwt-node so there have been quite a few changes. Sorry for the trouble, do you think you can sync this PR back up with master?

jpuri · 2018-01-10T03:17:11Z

Hey @kennetpostigo , sure I will sync up the PR and work n feedbacks you shared. Thx.

jpuri · 2018-01-10T16:25:07Z

Hey @kennetpostigo , point-1 above is not clear to me: output after diffing both paths should be relative to the current working directory. I also checked and compared with NodeJS output and you are right its different.

I have implemented 2, 3 and I am working on improving test coverage. More tests are required - PR is not complete. I requested a review so that I would know that my approach is correct.

kennetpostigo · 2018-01-10T17:25:37Z

@jpuri after messing around in the node repl, the original thought was wrong, the reason why the output is different is because in node.js it expects the input to be directories and not files, so if you change:

path.relative("bar/index.html", "index.html")

to

path.relative("bar", "index.html")

you will get "../index.html".

Personally I prefer the manner in which it is implemented now, it feels more precise to me. However, if porting code directly from node.js to lwt-node this might be one of those "gotchas". I leave the decision up to you of whether you want to leave it as is, or change it to match node.js.

jpuri · 2018-01-10T18:37:32Z

That was my thought also that nodejs is expecting directory and not file.
The current implementation as I have done will break if user passes not file-path but directory. I would prefer to keep this function same as node's. But it may be useful to have another like relativeFilePath.

kennetpostigo · 2018-01-10T19:10:53Z

@jpuri thats a good idea! 😃

jpuri · 2018-01-11T18:25:32Z

Path.resolve([""]) should return current working directory. This is not the case atm, is this bug or intended.

kennetpostigo · 2018-01-11T23:31:28Z

@jpuri good catch, it's definitely not intended behavior and oversight on my part, the bug fix is probably:

let resolve = paths => {
  let keepJoining = ref(true);
  let rePath = ref("");
  let onPath = (path, index, e) =>
    e ?
      {
        keepJoining := false;
        rePath := path ++ sep ++ rePath^;
      } :
      (
        switch path {
        | ""
        | "./"
        | ".\\" => Sys.getcwd() /* <----- fix*/
        ...

Would you want to add this into your current PR?

jpuri · 2018-01-13T13:12:26Z

PR looks complete to me now, also I added change and test coverage for resolve method.

kennetpostigo · 2018-01-14T05:00:08Z

@jpuri thank you! I'll take a look at this ASAP :)

jpuri · 2018-01-14T12:08:28Z

src/renodeUtils.re

@@ -6,6 +6,47 @@ let charSep =
  | _ => '/'
  };

+let rec removeCommonPath = (~path1, ~path2) => {


These switches can be replaced by ternary operators may be, but they are best at handling exceptions. Plz share if you have better suggestions about code here.

I personally prefer switches, they make refactoring easier and more explicit the exceptions that are being handled. These look great!

kennetpostigo · 2018-01-16T15:15:17Z

The changes look great! Thank you for your work! CI is only failing on macOS b/c travis cancels the build if theres no ouput after a period of time, but linux builds and passes. Feel free to squash and merge @jpuri

jpuri · 2018-01-17T17:43:51Z

Hi @kennetpostigo, I have squashed the changes, can you plz merge as I do not have that option enabled.

jpuri force-pushed the master branch from 8091088 to 8befa6b Compare January 9, 2018 17:23

kennetpostigo requested changes Jan 9, 2018

View reviewed changes

jpuri force-pushed the master branch from 3184424 to 4b99dac Compare January 13, 2018 13:10

jpuri force-pushed the master branch 2 times, most recently from 6c94f37 to 922ce24 Compare January 14, 2018 10:02

jpuri commented Jan 14, 2018

View reviewed changes

kennetpostigo approved these changes Jan 16, 2018

View reviewed changes

Adding relative method in path interface

bc206ac

jpuri force-pushed the master branch from 922ce24 to bc206ac Compare January 17, 2018 17:42

kennetpostigo merged commit 6875006 into kennetpostigo:master Jan 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding relative method in path interface #5

Adding relative method in path interface #5

jpuri commented Jan 6, 2018

kennetpostigo commented Jan 6, 2018 •

edited

Loading

jpuri commented Jan 7, 2018

kennetpostigo commented Jan 7, 2018

jpuri commented Jan 7, 2018

kennetpostigo commented Jan 7, 2018

jpuri commented Jan 7, 2018

kennetpostigo commented Jan 7, 2018

jpuri commented Jan 7, 2018

jpuri commented Jan 8, 2018

kennetpostigo commented Jan 8, 2018

jpuri commented Jan 9, 2018

kennetpostigo commented Jan 9, 2018

andreypopp commented Jan 9, 2018 •

edited

Loading

kennetpostigo commented Jan 9, 2018

kennetpostigo left a comment •

edited

Loading

kennetpostigo commented Jan 10, 2018

jpuri commented Jan 10, 2018

jpuri commented Jan 10, 2018

kennetpostigo commented Jan 10, 2018 •

edited

Loading

jpuri commented Jan 10, 2018 •

edited

Loading

kennetpostigo commented Jan 10, 2018 •

edited

Loading

jpuri commented Jan 11, 2018

kennetpostigo commented Jan 11, 2018 •

edited

Loading

jpuri commented Jan 13, 2018

kennetpostigo commented Jan 14, 2018

jpuri Jan 14, 2018

kennetpostigo Jan 16, 2018

kennetpostigo commented Jan 16, 2018 •

edited

Loading

jpuri commented Jan 17, 2018

Adding relative method in path interface #5

Adding relative method in path interface #5

Conversation

jpuri commented Jan 6, 2018

kennetpostigo commented Jan 6, 2018 • edited Loading

jpuri commented Jan 7, 2018

kennetpostigo commented Jan 7, 2018

jpuri commented Jan 7, 2018

kennetpostigo commented Jan 7, 2018

jpuri commented Jan 7, 2018

kennetpostigo commented Jan 7, 2018

jpuri commented Jan 7, 2018

jpuri commented Jan 8, 2018

kennetpostigo commented Jan 8, 2018

jpuri commented Jan 9, 2018

kennetpostigo commented Jan 9, 2018

andreypopp commented Jan 9, 2018 • edited Loading

kennetpostigo commented Jan 9, 2018

kennetpostigo left a comment • edited Loading

Choose a reason for hiding this comment

kennetpostigo commented Jan 10, 2018

jpuri commented Jan 10, 2018

jpuri commented Jan 10, 2018

kennetpostigo commented Jan 10, 2018 • edited Loading

jpuri commented Jan 10, 2018 • edited Loading

kennetpostigo commented Jan 10, 2018 • edited Loading

jpuri commented Jan 11, 2018

kennetpostigo commented Jan 11, 2018 • edited Loading

jpuri commented Jan 13, 2018

kennetpostigo commented Jan 14, 2018

jpuri Jan 14, 2018

Choose a reason for hiding this comment

kennetpostigo Jan 16, 2018

Choose a reason for hiding this comment

kennetpostigo commented Jan 16, 2018 • edited Loading

jpuri commented Jan 17, 2018

kennetpostigo commented Jan 6, 2018 •

edited

Loading

andreypopp commented Jan 9, 2018 •

edited

Loading

kennetpostigo left a comment •

edited

Loading

kennetpostigo commented Jan 10, 2018 •

edited

Loading

jpuri commented Jan 10, 2018 •

edited

Loading

kennetpostigo commented Jan 10, 2018 •

edited

Loading

kennetpostigo commented Jan 11, 2018 •

edited

Loading

kennetpostigo commented Jan 16, 2018 •

edited

Loading