Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Script to redeploy testlib.sml to all exercises on changes. #206
Script to redeploy testlib.sml to all exercises on changes. #206
Changes from 6 commits
156c591
627c2dc
110d8d7
721dd7b
32774e5
f276a74
d4c26df
48de3e8
f75f658
57b3885
38dff0e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space in
dir = (Path.concat (base,slug)),
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What space? I don't have a space between
x
and[]
in line 6,File.access (x,[])
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for consistency with testlib one could write
f(x, y)
instead off(x,y)
everywhere. But think it is not important. Consistency can also be a pain :D.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered changing it in case it is confusing, but using
'
as aprime
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 itins
which was a close second in my consideration. I supposeinstream
isn't too long though. I don't know why I felt so averse to a longer name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usingin
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)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 itinstream
orinStream
. I am at work right now, so I won't get to it until later.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 .There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space in
fun relUnixPathToAbs x=
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concept
is unused, so I would delete it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space in
relUnixPathToAbs"exercises/practice"
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind writing
List.app
instead ofapp
? 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 withapp
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment basically restating the name of the script seems pretty redundant to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I am fine with that, I was just a little bit worried if somebody else might wonder what exactly "redeploy" might mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄