Use proper type for file permissions

[felixSchl]

This allows for zero-padded modes, such as e.g. 0777, which is the default for directories.

This is in line with the javascript version which takes a string.

[michaelficarra]

I really don't want arbitrary strings used in these interfaces. We can do better than the nodejs API. Something kind of like this:

type Perm = { r :: Boolean, w :: Boolean, x :: Boolean }
type Perms = { u :: Perm, w :: Perm, o :: Perm }

permFromStr :: String -> Maybe Perm
permFromStr "0" = Just { r: false, w: false, x: false }
-- ...

permsFromStr :: String -> Maybe Perms
permsFromStr (u : w : o : []) = mkPerms <$> permFromStr u <*> permFromStr w <*> permFromStr o
  where mkPerms u w o = { u: u, w: w, o: o }
permsFromStr ("0" : xs) = permsFromStr xs
permsFromStr _ = Nothing

[felixSchl]

I do tend to agree, we just need a show and read instance for some Perms data type as you have outlined. Are you working on this? I would be interested to contribute this.

[michaelficarra]

I'd love to, but I'm too busy right now. Go ahead and take it.

[michaelficarra]

Oh also, don't use show and read in that way. Use permsFromStr and permsToStr functions like I did above. Show and read should be used for converting a value to/from its representation in PureScript code.

[felixSchl]

I wrote a module dedicated to permissions and exported functions permsFromString, permsToString, permsToNum and the two opaque data types Perm and Perms.

permsFromString returns a value wrapped in Maybe, whereas permsToString and permsToNum return concrete values. permsToNum is a bit of an odd one, since in theory parseInt can fail (return NaN) but because we completely control the numbers passed to it inside the module, we know it won't fail.

I have not yet plugged this into the actual functions mkdir and so on. Do you think we should expect the user to pass a Perms type to these functions or a string / number and we use the Perms internally? I am personally for requiring a Perms type as parameter, so we can be assured it's a valid value.

[felixSchl]

Also, I did end up adding a show instance for representing the Perms type, it will render the permissions as ls -l would. Records apparently do not automatically derive Show.

[michaelficarra]

I like it. I would drop the Show instances, though.

[felixSchl]

I had a feeling you would say that, but I would argue that it's convenient to be able to view the value for debugging purposes. It would be great if record types just had automatic show instances, given that all it's fields have instances, too. Then it could just be rendered a-la JSON.stringify. Maybe the show function could just be permsToString?

What do you reckon in terms of mkdir taking a Perms as parameter? Seems like the logical way to go, right? Alternatively, we could keep Number for compatibility's sake and internally use Perms to convert to the correct value.

[michaelficarra]

It would be great if record types just had automatic show instances

Yep, see https://github.com/purescript/purescript/issues/970.

What do you reckon in terms of mkdir taking a Perms as parameter?

Yes, we should accept Perms as input. Number and String should not be used to represent permissions in any PureScript programs.

[felixSchl]

Alright, I added that. I don't know if I like it too much though because I had to drop in fromJust, since mkdir calls mkdir' with default permissions 777.

[michaelficarra]

Just don't use permsFromString. Use the constructors directly, or if exporting the constructors is not desirable, export the mkPerm and mkPerms functions.

[michaelficarra]

It also may be useful for the Node.FS.Perms module to export some common Perms values such as 0644, 0600, and 0777.

[felixSchl]

I made Perm an instance of Semigroup, so that we can compose permissions additively like this, e.g.: (r <> x) for read and execute flags to be set, i think it looks cleaner. We could make it a Monoid, too, but that's another added dependency. Might be cleaner than exporting the random none.

[michaelficarra]

Great idea. :+1:

[felixSchl]

Thanks for the feedback, I made the changes as suggested, but not sure about what you mean that now we can drop mkPerms, - even if we use newtypes, mkdir will still need it. Or do you mean exporting the constructor and using it directly to refer to the record fields by name?

Maybe use Int now that we have it?

We have no dependency on Data.Int, i.e. purescript-integers. If that's something we want, can do.

[garyb]

Adding the purescript-integers dependency isn't unreasonable, it will soon be included almost everywhere transitively via things like purescript-control and purescript-arrays anyway.

[felixSchl]

Alright, I moved to Data.Int and added the dependency. You may want to review this line, because if converting to string for usage other than printing to screen is not the purescript way of doing things, my use of show may not be the best approach. Also refer to purescript/purescript-integers#5.

[felixSchl]

By the way, is there a standard type class that would suite to remove a permission flag?

[hdgarrood]

Perhaps (-) from Ring?

[hdgarrood]

On second thoughts, I'm not sure if it's possible to make a sensible Semiring instance for Perms. (What would multiplication represent?)

Anyway, this looks good to me, cheers :) I squashed your commits and merged into master manually.

[michaelficarra]

@hdgarrood (*) is bitwise-and (a filter), and (+) is bitwise-or. I'll send a PR.

[hdgarrood]

Ah, of course!

[michaelficarra]

Turns out (-) is implication! https://github.com/purescript-node/purescript-node-fs/pull/15