Script to redeploy testlib.sml to all exercises on changes. #206

guygastineau · 2022-06-12T15:50:50Z

Okay, so I decided it is better to factor this redeploy script out of #205 .

I also went ahead and refactored to use the Makefile for ease of use and hopefully for better portability across operating systems. GH inscluded my commit message below.

The directory name sml-bin makes the most sense to me for these sml
"scripts". Adding them to the makefile also makes it even easier to
use these than it is when using "#! /usr/bin/env -S poly --script" as
a shebang line. Consequently this should be portable to any Windows
system that has GNU make installed, so it is probably better than the
shebang. In the code I used the path library to help make things OS
portable, but I haven't ever used SML on Windows, so it is untested
for that enironment.

The directory name `sml-bin` makes the most sense to me for these sml "scripts". Adding them to the makefile also makes it even easier to use these than it is when using "#! /usr/bin/env -S poly --script" as a shebang line. Consequently this should be portable to any Windows system that has GNU make installed, so it is probably better than the shebang. In the code I used the path library to help make things OS portable, but I haven't ever used SML on Windows, so it is untested for that enironment.

rainij

I am still thinking about sml-bin vs bin. My first impulse says that the redeploy script could perfectly live inside bin. Can you explain in more detail why you put it somewhere else?

I suggested some changes, let me know what you think.

I did not have time to have a closer look into the redeploy script (I locally checked that it seems to work though). Maybe I have time for this soon, but if you agree on my comment on usage of open I would wait for you to change that before having a second look.

rainij · 2022-06-12T16:57:04Z

Makefile

@@ -15,4 +15,7 @@ test-%:
 	@cd ./exercises/practice/$(exercise) && cat test.sml | sed 's/$(exercise).sml/.meta\/example.sml/' | poly -q
 	@echo

+redeploy-testlib:
+	poly --script sml-bin/redeploy-testlib.sml
+
 .PHONY: test


redeploy-testlib should be .PHONY too

Oh, cool. I never knew what .PHONY does.

I read up on it. Yes, this makes perfect sense, and it is now done.

rainij · 2022-06-12T17:00:38Z

sml-bin/redeploy-testlib.sml

+
+    val testLib = fileBytes "lib/testlib.sml";
+in
+    app (writeFileBytes testLib o testLibName practice) (dirList practice)


I think a very short docstring explaining what the script does might be a good idea (maybe at the top of the file). Of course the filename essentially says it but I would still prefer a short docstring. Let me know if you think this would not be helpful.

A comment basically restating the name of the script seems pretty redundant to me.

OK I am fine with that, I was just a little bit worried if somebody else might wonder what exactly "redeploy" might mean.

Hmm, well, I often take for granted that English is my first language, aber meine Meinung nach sollen wir die Problemen sprachlicher Mehrdeutigkeit der Nicht-Muttersprachlichern beachten 🙃

So, maybe it is best for there to be some more direct comments in the script 😄

rainij · 2022-06-12T17:05:53Z

sml-bin/redeploy-testlib.sml

+        , file = "testlib.sml"
+        }
+
+fun dirList dirname = let


Can you make the formatting more consistent with the other sml-files (e.g. testlib.sml)? E.g. always indent by two spaces and for example

fun foo = let val a = ... in do_something ... end; (* Note: there is a semicolon. *)

instead of

fun foo = let val a = ... in do_something ... end;

I have the feeling that this is better. But feel free to criticize this :) .

Sure. I'll make some changes. I need to figure out how to set sml-mode to use 2 spaces. It's indentation is worse than indentation in most emacs major modes. There really shouldn't be semicolons at the end of function definitions like this. Semicolons are for sequencing typically imperative actions. I had them in the first version of this script, but I think we should avoid them except where they are meaningful.

Personally, I don't think there is an issue with let being on the opening line of the function, but I don't mind changing it for consistency.

I write a lot of haskell at work, so I am addicted to commas to the left in records and lists ;p

OK I am fine with omitting the semicolons. I was used to them since in the Book "ML for the working programmer" the author uses them wherever they are allowed (not just in the REPL or for imperative actions). But he actually states somewhere that they are optional there. It seems to be a matter of taste. I followed the author in my code, since I was used to something similar in javascript: In js "most of the time" the semicolons are not relevant, but not always, and therefore the general advice for js is to put them everywhere (I know javascript is probably not the most well designed language :D).

So since testlib already omits them we should omit them everywhere.

Yeah, with JS including the semicolons makes sure everything works as intended. With SML, including semicolons can change the meaning of a program 🤯

rainij · 2022-06-12T17:07:38Z

sml-bin/redeploy-testlib.sml

@@ -0,0 +1,45 @@
+open OS.Path;
+open OS.FileSys;


I would prefer not to use open. Makes some things easier. E.g. when I want to know what access does I first have to find out where it comes from (OS.Path or OS.FileSys, probably not difficult to guess but still easier if it is explicit). What do you think?

Since this was a "script", especially originally, I figured using open should be fine. I am not against using qualified names though. I will probably make more convenient names aliasing the structures.

I switched everything to qualified names now.

The aliases are a good compromise :) .

rainij · 2022-06-12T17:22:17Z

@guygastineau this PR is related to issue #110. Not sure if you are aware of it.

guygastineau · 2022-06-12T21:15:46Z

I hadn't seen #110. Thank you for referencing it for me.

I had set it to 775 when it had a shebang and lived in `bin`. Trying to execute it will fail, so it shouldn't be executable.

guygastineau · 2022-06-12T21:22:50Z

Concerning not using bin for sml "scripts". When I see something in a bin dir I expect that I can execute it from the project root like this bin/something. Following the principle of least astonishment, I think it is best for such sml code to live in another directory. It could be named very many things including code, sml, etc..., but I definitely think all files in bin should be executable.

I shouldn't have combined to actions but I did. I removed all unnecessary semicolons while refactoring to use qualified names. Sorry.

guygastineau · 2022-06-12T21:38:54Z

I'll set my editor for using 2 spaces in sml-mode, and then I can reformat and push the changes.

I also found and removed two more unnecessary semicolons.

rainij · 2022-06-13T14:46:16Z

Concerning not using bin for sml "scripts". When I see something in a bin dir I expect that I can execute it from the project root like this bin/something. Following the principle of least astonishment, I think it is best for such sml code to live in another directory. It could be named very many things including code, sml, etc..., but I definitely think all files in bin should be executable.

@guygastineau sounds reasonable to me. But I wonder if sml-bin is a good name then. Because now I would think that sml-bin still contains "executables", which happen to be written in SML. Maybe sml-scripts? But actually I never thought about the question what a "script" really is^^. Similar vor "bin" - sounds like machine code to me but I usually don't care if it is machine code ... .

In any case, it is interesting that "this" problem arises from the desire to support Unix and Windows. I am not very happy that this is the reason for the additional folder, but I see your point.

So in the end I have some doubts but I would be OK with or without the additional folder. I have a slight preference to still put the script into 'bin' since although I understand your reasoning I think it still has a drawback. Others not knowing your reasoning (like me before reading your justification) might now struggle with the question where to put the next script. I must admit that I have no clue what others might expect over "bin" but at least I have seen non-executables there often. However this might be just the case since I often work with engineers and I for myself did not work in IT (until 4 years ago) and my educational background is far away from IT and Computer Science.

But I have no strong opinion on that I just shared my thoughts in case anybody is interested. So I decide to stay neutral on this topic from now.

rainij

@guygastineau I think the README should refer to the policy mentioned in #110. When I first visited the repo it was not clear to me if every exercise should have its unique copy of testlib (with possible custom changes) or not. So it might be worth noting it more clearly.

rainij

Now I had some time for the redeploy script. I like it. I am certainly no SML Guru but I tried to give some feedback which hopefully improves the script a tiny bit.

rainij · 2022-06-13T16:34:01Z

sml-bin/redeploy-testlib.sml

+    val concept = relUnixPathToAbs "exercise/concept"
+    val practice = relUnixPathToAbs"exercises/practice"
+    val testLib = fileBytes "lib/testlib.sml"
+  in


Would you mind writing List.app instead of app? I am usually explicit with these things (because I hate puzzling out what this is if I come back after some time of absence from some particular topic like SML). But it is a question, so feel free to stay with app.

rainij · 2022-06-13T16:34:30Z

sml-bin/redeploy-testlib.sml

+val _ =
+  let
+    val concept = relUnixPathToAbs "exercise/concept"
+    val practice = relUnixPathToAbs"exercises/practice"


Missing space in relUnixPathToAbs"exercises/practice".

rainij · 2022-06-13T16:35:25Z

sml-bin/redeploy-testlib.sml

+
+val _ =
+  let
+    val concept = relUnixPathToAbs "exercise/concept"


concept is unused, so I would delete it.

rainij · 2022-06-13T16:36:04Z

sml-bin/redeploy-testlib.sml

+    BinIO.closeOut out
+  end
+
+fun relUnixPathToAbs x= Path.concat ((File.getDir ()), Path.fromUnixPath x)


Missing space in fun relUnixPathToAbs x= .

rainij · 2022-06-13T16:44:11Z

sml-bin/redeploy-testlib.sml

+
+fun fileBytes path =
+  let
+    val in' = BinIO.openIn path


Is it some well-established convention to call this in'? Looks a little bit awkward to me, especially since my IDE (vscode) and even github recognize the first two characters as a keyword. Would you mind calling it differently?

Actually I have checked that my emacs with SML mode does not have this issue, but even there I would prefer not to call it in'.

I considered changing it in case it is confusing, but using ' as a prime like identifier modification is pretty common in the ML family. For the record, I think the fact of general tooling being incapable of recognizing/capturing the expressiveness of a language is a poor reason to limit one's expressiveness when using the language. If I rename it I am likely to name it ins which was a close second in my consideration. I suppose instream isn't too long though. I don't know why I felt so averse to a longer name.

I agree that bad tooling is not the best reason for a change, but this was just for information. I have nothing against prime in general but here it seems to be there only because using in is not legal and I feel this is no good reason either. Most of the time when I encountered a "primed" variable, then there also was a related variable without a prime.

My question about conventions was more specifically about using the exact string in' as a whole (not just something with a prime at the end).

So if you already considered it to be confusing why not change it to ins (wouldn't be my choice, but I would be fine with it)?

Yes, you are right. Prime here is not really really semantically consistent, but rather it is a syntactic hack to get around not having my ideal identifier. Sorry, I meant to make my last comment sound more positive about changing the name of the declaration. Oat likely I will call it instream or inStream. I am at work right now, so I won't get to it until later.

The longer name of instream would be preferable to any of the shorter names. You could still use the ' to indicate the additional information stated @guygastineau .

Thanks, @kotp. Yes, you are right about instream. I have pushed it now. ' didn't have any meaning in my usage which is why it was wrong. I was using a more or less semantic naming convention as a hack to get around the inconvenience of a keyword.

rainij · 2022-06-13T16:44:40Z

sml-bin/redeploy-testlib.sml

+fun testLibName base slug =
+  Path.joinDirFile
+    {
+      dir = (Path.concat (base,slug)),


Missing space in dir = (Path.concat (base,slug)),.

What space? I don't have a space between x and [] in line 6, File.access (x,[]).

Maybe for consistency with testlib one could write f(x, y) instead of f(x,y) everywhere. But think it is not important. Consistency can also be a pain :D.

Space after comma,after all,is helpful in reading,so they say. I do not know why,in any instance,one would not use a space,every time,after a comma.

I got used to it, because a lot of old SML doesn't use spaces in tuples. Of course, a lot of old SML tries to immitate c-style function calls by excluding a space between uncurried function names and their product argument; this convention really annoys me, so I don't need to stick with the other convention for a partial tradition either. I will change it if you all think it is better.

It is the difference of my eyes seeing the comma, and not seeing it. So white space in code, when not syntactically limited to white space breaking things when there can definitely benefit from having it.

rainij · 2022-06-13T16:50:07Z

@guygastineau I am done with my review. When my suggestions are addressed or at least discussed away^^ I will certainly approve this PR. I hope this saves some time for the real maintainers when they do their review.

I wrote a brief note about keeping testlib.sml in sync for all exercises in the track, and I mentioned 'make redeploy-testlib' as the provided tool for handling this requirement. I did not elaborate on a policy, because we still don't really have one. Further work related to exercism#110 should result in a clear policy, and at that point the README.md might need some more updating.

guygastineau · 2022-06-14T20:45:58Z

Geeze, I accidentally started a review, so some of my comments over a few days got bunched up. I just hit submit review to get them to go out. Some of them might have been out of date. Anyway, I think we probably have this ready to go.

rainij · 2022-06-15T17:51:57Z

@guygastineau I am fine with everything. I approve :) .

Maybe somebody else can give a third opinion on the discussion on bin vs sml-bin vs something else.

README.md

kotp · 2022-06-16T03:57:55Z

Maybe somebody else can give a third opinion on the discussion on bin vs sml-bin vs something else.

bin/ is used to hold executables, so if that is what is in there, the sml track itself is the context of what kind of executables they would be, so sml-bin would be, probably, considered redundant naming.

ErikSchierboom

Approving to allow the PR to be merged.

Note that I do also support bin over sml-bin, but I don't massively care.

guygastineau · 2022-06-16T12:47:53Z

I can change it to bin, but I would like to clarify.

I am not sure that sml-bin is the best name, but I should be clear that redeploy-testlib.sml is not what sml would consider an executable. With mlton we would make an executable like this mlton sml-bin/redeploy-testlib.sml, and this would produce an executable ELF. With PolyML we can also make a standalone executable using polyc sml-bin/redeploy-testlib.sml, although this will fail since the effects are not packed into a function named main. smlnj has several other ways to make executables. With PolyML there is another way to make a special shebang, (which I did first) #! /usr/bin/env -S poly --script, and this lets you execute the "script" directly.

For track tools like redeploy-testlib.sml I see no reason to force maintainers and contributors to compile the code to executable binaries, and the shebang method of turning the code into a script seems a kludge at best, and I don't expect it to be portable for Windows. So, using the Makefile seems like it hits the sweet spot of usability, comprehensibility, and parsimony to me.

I do sometimes have directories called bin in my projects where the contents aren't executable files. In all cases (for me), these are projects using complex build systems like cabal or cargo, and the files in bin are source that will be compiled into executable files that go somewhere else, ie. not back into bin. In this project, as with other exercism projects, the bin directory is more general and typically used for executable files as far as I have experienced. For example, the venerable fetch-configlet is usually found in bin, and after its use we find the static binary configlet appropriate for our local environment in bin as well.

What we are really talking about with sml-bin is a directory with track maintenance tools written in sml such that they can be loaded by an implementation and executed as read. This is very similar to the way that lisp implementations might load a script. Any attempt at calling the files directly will result in an error from your shell, because the source files are not executable. Not only does this approach mean we don't have to worry about shebangs concerning windows compatibility, but using make to drive these tools means we can add means for maintainers to configure the system to use different sml implementations. I am toying with this on the side, and several of my side projects from last year are building/running with poly, smlnj, and mlton. This kind of flexibility is not the purpose of this PR, but I mention this, because the makefile approach will help facilitate this kind of flexibility in the future.

What am I trying to say?

The kind of files in sml-bin are not like the kind of files in bin. bin/generate is executable. bin/fetch-configlet is executable. bin/configlet is executable.
Maybe (likely) sml-bin is not the best name. If so, we should find a better name now before merging the PR. Maybe tools is a better name (or track-tools, etc...).
Lumping these non-executable files, that sml implementations also wouldn't consider executable, into the existing bin directory with clearly executable files is messy and confusing in my opinion. That is the reason for my preference for another directory.

So, if everyone here thinks we should mix the files in bin, then I will just go along with it, but I think there are good reasons for separating them. If we determine, as I suspect, that sml-bin is not the best name, then I think it is worth it for us to figure out a better name for these tools written in sml and used through the makefile.

I hope I have made my rationale clear. I look forward to your reactions.

Co-authored-by: Victor Goff <[email protected]>

kotp · 2022-06-16T15:56:49Z

So, if everyone here thinks we should mix the files in bin, then I will just go along with it, but I think there are good reasons for separating them.

I am against mixing non-executable files in bin/.

The Linux directory structure shows how general directory structure is for an arguably larger project. For example; using usr/ is for some executables and supporting data. So perhaps usr/ is a good place for these things.

ErikSchierboom · 2022-06-17T06:59:46Z

So, if everyone here thinks we should mix the files in bin, then I will just go along with it, but I think there are good reasons for separating them.

You've made a good argument for separating them.

guygastineau · 2022-06-19T12:58:33Z

The Linux directory structure shows how general directory structure is for an arguably larger project. For example; using usr/ is for some executables and supporting data. So perhaps usr/ is a good place for these things.

Hmm, I read over the LDS documentation again. I never see files in usr at the top level, rather I see subdirectories like bin, sbin, local, lib, and share. Given how much language in those docs is specific to operating systems I am hesitant to use usr as the home for these files.

Still, I don't think that sml-bin is the best name. I don't think it is catastrophic as a name for the directory, but I would like to do better. Since they are sort of acting like scripts and are getting loaded by an SML implementation, maybe we can call it scripts?

guygastineau · 2022-06-19T21:28:24Z

By the way, I don't know if you all care enough to debate potential names. Please do if you want to. I see no reason to rush through these contributions, but I would rather not let it languish needlessly either. I will give a couple days for you all to chime in, but provided silence I will go ahead and merge sometime on Tuesday with the current name.

kotp · 2022-06-19T23:55:39Z

The scripts name might be communicative.

ErikSchierboom · 2022-06-20T06:20:23Z

Yeah, scripts sounds good!

Initially, I thought it must be clear enough from the title/name of the script itself, but on further reflection, a documenting comment might help a non-native english speaker avoid the difficulties of ambiguity.

guygastineau · 2022-06-20T13:56:55Z

Thank you all for the thorough and thoughtful reviews. We are now using scripts instead of sml-bin which I think is a positive move. I also added a documenting comment to the redeploy script.

@rainij despite an admitted dearth of experience with SML you gave an excellent review. It is a testament to your abilities as an engineer, and I expect the advanced degree in mathematics plays some role in your meticulous contributions to the conversation. Thank you very much.

guygastineau · 2022-06-20T13:59:07Z

It looks like we do need an SML maintainer or anyone else with write access here to merge the PR.

kotp · 2022-06-21T18:15:32Z

This was merged a day earlier than I thought was indicated. But looks good @guygastineau . Just had time to do a last check based on the timeline information I had.

guygastineau · 2022-06-21T19:39:59Z

This was merged a day earlier than I thought was indicated.

Sorry @kotp , I meant I would leave it for a few days to wait for discussion about renaming sml-bin. Given a productive discussion ensued with an acceptable outcome for all participants, I considered the matter settled.

guygastineau · 2022-06-21T19:43:05Z

By the way, I don't know if you all care enough to debate potential names. Please do if you want to. I see no reason to rush through these contributions, but I would rather not let it languish needlessly either. I will give a couple days for you all to chime in, but provided silence I will go ahead and merge sometime on Tuesday with the current name.

I think this comment of mine is where our miscommunication started, @kotp although rereading your comment stating that scripts might be communicative I realized that I assumed that was a colloquial agreement on the new name, but you might have meant you thought it warranted more discussion. I am sorry if I merged before our discussion was actually over.

kotp · 2022-06-21T20:08:42Z

It was just that you said "for a couple of days" and so I came back to do a final "after changes" review. It's all good. Would rather have it in, it is an improvement for sure. Just some things I know I have time to come back to, even if I find out that I knew no such thing. ;)

guygastineau mentioned this pull request Jun 12, 2022

nearTo fix #205

Merged

rainij requested changes Jun 12, 2022

View reviewed changes

Change file perms to 664 for sml-bin/redeploy-testlib.sml

627c2dc

I had set it to 775 when it had a shebang and lived in `bin`. Trying to execute it will fail, so it shouldn't be executable.

guygastineau added 2 commits June 12, 2022 17:24

Make redeploy-testlib phone as well.

110d8d7

Use qualified names.

721dd7b

I shouldn't have combined to actions but I did. I removed all unnecessary semicolons while refactoring to use qualified names. Sorry.

Format redeploy-testlib.sml like teslib.sml

32774e5

I also found and removed two more unnecessary semicolons.

guygastineau mentioned this pull request Jun 12, 2022

Improve and distribute epsilon-delta test function across exercises #110

Open

4 tasks

rainij requested changes Jun 13, 2022

View reviewed changes

guygastineau added 2 commits June 14, 2022 16:31

Formatting, names, parentheses.

d4c26df

rainij approved these changes Jun 15, 2022

View reviewed changes

kotp reviewed Jun 16, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

ErikSchierboom approved these changes Jun 16, 2022

View reviewed changes

guygastineau and others added 2 commits June 16, 2022 09:13

Update README.md

48de3e8

Co-authored-by: Victor Goff <[email protected]>

Add spaces inside tuples.

f75f658

guygastineau added 2 commits June 20, 2022 09:50

Use scripts directory instead of sml-bin.

57b3885

Add a documenting comment to redeploy-testlib.sml.

38dff0e

Initially, I thought it must be clear enough from the title/name of the script itself, but on further reflection, a documenting comment might help a non-native english speaker avoid the difficulties of ambiguity.

ErikSchierboom merged commit c14b2f3 into exercism:main Jun 20, 2022

Script to redeploy testlib.sml to all exercises on changes. #206

Script to redeploy testlib.sml to all exercises on changes. #206

Conversation

guygastineau commented Jun 12, 2022

rainij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guygastineau Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainij commented Jun 12, 2022

guygastineau commented Jun 12, 2022

guygastineau commented Jun 12, 2022

guygastineau commented Jun 12, 2022

rainij commented Jun 13, 2022

rainij left a comment

Choose a reason for hiding this comment

rainij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainij commented Jun 13, 2022

guygastineau commented Jun 14, 2022 • edited Loading

rainij commented Jun 15, 2022

kotp commented Jun 16, 2022

ErikSchierboom left a comment

Choose a reason for hiding this comment

guygastineau commented Jun 16, 2022

kotp commented Jun 16, 2022

ErikSchierboom commented Jun 17, 2022

guygastineau commented Jun 19, 2022

guygastineau commented Jun 19, 2022

kotp commented Jun 19, 2022

ErikSchierboom commented Jun 20, 2022

guygastineau commented Jun 20, 2022

guygastineau commented Jun 20, 2022

kotp commented Jun 21, 2022

guygastineau commented Jun 21, 2022

guygastineau commented Jun 21, 2022

kotp commented Jun 21, 2022

guygastineau Jun 13, 2022 •

edited

Loading

guygastineau commented Jun 14, 2022 •

edited

Loading