Introduce a unix module and implement $unix:umask

[krader1961]

Resolves #681

[krader1961]

@xiaq Do you prefer people push PR updates as deltas (i.e., standalone commits) or forced pushes after squashing the update into the previous commit? I'm asking because I noticed you merged a couple of my earlier PRs without squashing the subsequent change(s) I made in response to your review feedback. Which is not how other projects typically handle pull-request deltas made as a result of review feedback.

[krader1961]

@xiaq, I've addressed all of your points with one exception. You made two contradictory recommendations with regarding to the unix: namespace on non-UNIX platforms. I have tentatively decided it makes more sense to make the namespace visible on non-UNIX platforms. Adding a $unix:usable variable makes it easy, and clear, to determine if that namespace is usable. The original approach of creating an empty namespace required doing == 0 (count (keys $unix:)) to determine if the namespace was usable. The $unix:usable var means any Elvish program can safely do use unix then if (not $unix:usable) { echo error message; exit 123 }.

[krader1961]

My introduction of the $unix:usable bool is also meant, obviously, to set a precedent. This makes it easy for an Elvish program to determine if it is running on a non-UNIX platform without resorting to considerably more complicated mechanisms such as comparing the $platform:os var against a list of acceptable values. The intent is to make it possible to introduce other optional namespaces such as windows: (or win:) that Elvish programs can easily, and clearly, test if they are usable.

[xiaq]

Re. making it easy for Elvish code to decide whether the unix module is available: you can introduce a $platform:is-unix variable instead.

Re. preference of how you push code: I don't mind, since when merging a PR in GitHub you can optionally squash all the commits. When I see people pushing commits that say "address PR feedback" I'd use "sqush and merge"; when I see people force-push I'd use "rebase and merge".

[krader1961]

Re. making it easy for Elvish code to decide whether the unix module is available: you can introduce a $platform:is-unix variable instead.

SGTM. I chose the current solution because it localizes the var for determining the usability of a module to that module. But given the existence of the platform module it is also reasonable to place equivalent vars in that module. I'll create an independent PR to do that and also introduce a $platform:is-windows variable at the same time since those are the cases of greatest interest at this time.

[krader1961]

@xiaq, Please take another look. I've rebased on the recent changes to the master branch but otherwise the changes only addressed your most recent feedback.

[xiaq]

Thanks. There are still two pieces of feedback that haven't been fully addressed; I unresolved the relevant conversations.

[xiaq]

Almost good to go.

[krader1961]

It's correct, but a small nitpick: use a RWMutex to allow concurrent reads.

That would be incorrect in this instance since the Get() method mutates the state. Allowing concurrent readers means that one of them might see, and return, the transitory zero value.

[xiaq]

That would be incorrect in this instance since the Get() method mutates the state. Allowing concurrent readers means that one of them might see, and return, the transitory zero value.

Oops, you are right!

Please format the files with goimports. You can format all Go files with this Elvish command:

goimports -w **.go

If you don't like the option of running goimports when the editor saves files, I'd suggest that you set up some Git commit hook to run this.

[krader1961]

I ran goimports. Please squash merge.

P.S., I've gone ahead and added a write hook to my Vim config. The problem with that approach is that if I edit a Go source file using some other program then goimports won't automatically be run. How would you feel about adding a make style build target that runs goimports and prettier if they are available?

[krader1961]

Also, I just installed prettier and ran it via prettier --tab-width 4 --prose-wrap always --write **.md. It modified six files 😄

[xiaq]

@krader1961 I introduced a make style target, enjoy :)

A few more nitpicks on the PR, but I'll merge it anyway and address myself:

• The godoc comment for the package should say clearly that it introduces an Elvish namespace;
• The godoc comment block for an exported symbol should always start with the exported symbol itself.

Also, I am not sure what the test case in unix_test.go is supposed to do: currently it verifies that the unix: module has at most 1 symbol; that number will simply need to be bumped when it gets more symbols, which seems repetitive and not very useful to me.

Do you have a specific failure mode in mind that this test case is supposed to catch? I'll remove it if it's not clear why it is there.

[krader1961]

Also, I am not sure what the test case in unix_test.go is supposed to do: currently it verifies that the unix: module has at most 1 symbol; that number will simply need to be bumped when it gets more symbols, which seems repetitive and not very useful to me.

That's leftover from when I intended to make it possible to use unix even on non-unix systems. It simply ensures that the namespace isn't empty -- which it would have been on non-unix systems. Nuke it.