search

exercism / sml

Exercism exercises in Standard ML.
https://exercism.org/tracks/sml
MIT License
26 stars 34 forks source link

nearTo fix #205

Closed guygastineau closed 2 years ago

guygastineau commented 2 years ago

I made a script to redeploy lib/testlib.sml to all exercises. After testing that script I realized that Space Age is using an improved version of the test function nearTo which includes a delta. This version of nearTo looks like it is more coherent to me, so I deployed it to all exercises.

~I isolated my redeploy script in its own commit. Please read the commit message. Basically, I figured out how to use poly in a shebang. If we want to use the Makefile instead then I think there are some questions about where sml code should live that is neither a library nor an executable. Anyway, if using unixisms is contentious, then we can change it. If reviewers would prefer that that script gets submitted as its own PR that seems reasonable to me. I include the updated testlib here, because having this script in the project while not having all the testlib.sml files the same seems unsound to me, but I can be flexible.~

The aforementioned script has been pulled out and refactored #206 . This branch has been rebased on main. I still think this PR should merge after #206, but they shouldn't break each other, so I don't think the order really matters.

As shown in the commit messages nearTo was only used by the space age exercise, and it appeared twice in the README.md and once in bin/generate. Updating the "blessed" testlib.sml, the edits to the peripheral files, and the deployment of the new testlib were all isolated into their own commit. They probably want rebasing, but I figured it is fine for now.

guygastineau commented 2 years ago

What do you think @rainij ?

guygastineau commented 2 years ago

I amended the main comment. I just decided it was better to include the script as its own PR. That way it is easier for people to review the code and approach there and ask for changes.

rainij commented 2 years ago

@guygastineau I just started to review the PR before your refactored out the script. Would have been better if you had set the PR to "draft"-status. No big problem, but I think it is better to not touch the PR after requesting review and before anyone responded.

rainij commented 2 years ago

@guygastineau this PR is related to issue https://github.com/exercism/sml/issues/110. Not sure if you are aware of it.

guygastineau commented 2 years ago

The students need the testlib.sml too! All of the package systems I see for sml are for smlnj and mlton and require cmlb file compatibilities that PolyML doesn't have. I think distributing the test library with the exercises is the most sane solution we have. Here are a few reasons why:

  1. I think we should strive for supporting several implementations of SML. That is not our current reality, but it is not that hard to do. The scheme track does it, and I think we can figure out a way too.
  2. Choosing an existing packaging system for SML would mean removing support for PolyML which is the traditional SML implementation for this track. That seems unacceptable to me.
  3. The test library is not very big, and we will want to extend it to help the test-runner (when we get there) achieve version 2 test-runner interface compatibility. This means having the test library completely under our control is actually helpful :)

Anyway, I do sympathize with your position too. As you admit though, #206 makes it less painful. I think most of the pain for this track comes from a lack of tooling.

rainij commented 2 years ago

The students need the testlib.sml too! All of the package systems I see for sml are for smlnj and mlton and require cmlb file compatibilities that PolyML doesn't have. I think distributing the test library with the exercises is the most sane solution we have. Here are a few reasons why:

1. I think we should strive for supporting several implementations of SML.  That is not our current reality, but it is not that hard to do.  The scheme track does it, and I think we can figure out a way too.

2. Choosing an existing packaging system for SML would mean removing support for PolyML which is the traditional SML implementation for this track.  That seems unacceptable to me.

3. The test library is not very big, and we will want to extend it to help the test-runner (when we get there) achieve version 2 test-runner interface compatibility.  This means having the test library completely under our control is actually helpful :)

Anyway, I do sympathize with your position too. As you admit though, #206 makes it less painful. I think most of the pain for this track comes from a lack of tooling.

@guygastineau maybe this is a misunderstanding. I actually don't want to do such a drastic step to throw away PolyML and use MLB. What I mean with "package-like" is just to find a way to avoid the copies of the testlib, actually in a minimally invasive way. I could have been clearer :) . Moreover, for now I am find with the copies, no problem. I don't want to go of topic here, so I stop here ... .

But just one question: What exactly do you mean by "The students need the testlib.sml too!". I ask because I am afraid that I miss some essential point in the overal architecture of exercism. Since this is probably a response to my "I guess the sml-test-runner is the only thing which "needs" it this way (but I didn't look into it)" I will explain what I mean with that: When I looked into this repo for the first time I wondered why there are so many copies of the testlib. I wondered why not just refer to the one in lib from everywhere. So I searched for constraints coming from outside the repo and found the test-runner repo and this was enough to make sense to me. I suspected that this was the only real "hard" reason, if this is not the case I would like to know, therefor I ask.

rainij commented 2 years ago

This PR is achieves the goal to unify all copies of testlib and also avoids a large diff in the companion PR https://github.com/exercism/sml/pull/206. So I approve the PR.

guygastineau commented 2 years ago

What exactly do you mean by "The students need the testlib.sml too!".

TL;DR; Some people use the exercism cli tool to solve problems in their local environment.

The long answer

I think the confusion comes from only experiencing exercism v3. I started using exercism near the end of v1 and through v2. I have actually been involved less since the advent of v3. There were no online editors, and there was no automatic test running before v3. In fact, I am pretty sure the sml-test-runner doesn't even do anything (I haven't looked deeply into the repo). The test runners always seemed like a good idea, but, finally, I also think it is an improvement to have an online IDE available. However, I am not interested at all in using the online IDE; I hate practically every minute of editing source code without emacs. The traditional, and I think still quite popular, workflow for exercism revolves around using the exercism cli to download exercises before solving them with one's local environment and finally submitting a solution via the exercism cli.

So, unless leadership takes away the legacy workflow (that would create a lot of dissent I think), then we have to provide the student with all the files they need for testing when they download the exercises via the exercism cli tool.

Hopefully, my previous statement is now clear.

rainij commented 2 years ago

@guygastineau thanks for the clarification on the question why students need the testlib. Now I see. I actually only know the v3 workflow (I am very new here). I keep this in mind.

Off topic: I also do not edit anything in the Web IDE. But I like it since it prevents me from installing something special locally. I have a file locally and when I am done I just copy it into the WebIDE for testing. For small code snippets it is OK.

kotp commented 2 years ago

Off topic: I also do not edit anything in the Web IDE. But I like it since it prevents me from installing something special locally. I have a file locally and when I am done I just copy it into the WebIDE for testing. For small code snippets it is OK.

You are not the only one. A Web IDE will 99.99% of the time never be comfortable to work in, compared to your local favorite editor.

ErikSchierboom commented 2 years ago

In fact, I am pretty sure the sml-test-runner doesn't even do anything (I haven't looked deeply into the repo).

This is not correct though. The SML test runner is functional. See https://github.com/exercism/sml-test-runner/blob/main/bin/run.sh for its core functionality. You can also see that it is enabled in the track's config.json: https://github.com/exercism/sml/blob/main/config.json#L7

So, unless leadership takes away the legacy workflow (that would create a lot of dissent I think), then we have to provide the student with all the files they need for testing when they download the exercises via the exercism cli tool.

We'll definitely not be doing that. The CLI is a key part of Exercism and will keep on being supported. The online editor is there as an alternative, not a replacement.

guygastineau commented 2 years ago

@ErikSchierboom thank you for clarifying about the SML test runner. I am glad to be mistaken.

We'll definitely not be doing that. The CLI is a key part of Exercism and will keep on being supported. The online editor is there as an alternative, not a replacement.

This is as I expected. I meant the possibility of losing the cli as a sort absurs thing thatwil never happen, but I appreciate the strong stance regardless 🙂

guygastineau commented 2 years ago

Thank you all for your engagement in the PR. I should have some time this afternoon to get the rest of these reasonable requests implemented. I was feeling pretty calorie deficient yesterday, and I hope the tone of my communications didn't suffer too much.

ErikSchierboom commented 2 years ago

Not at all (at least IMHO). It's great that you're working on this and asking good questions!

guygastineau commented 2 years ago

Great news. When you test the redeploy, mind you need lib/testlib.sml from commit 6e5c7902b508a9a8ac83e23c7d1661c88a770be5. Otherwise, the diff will show that only space age got a new testlib. You probably already knew this, I just... am verbose sometimes.

kotp commented 2 years ago

@rainij The history now looks purposeful and curated. So this looks like it is ready to come in. After your local tests, I would reconsider if you want to do the squash. The commits might be purposefully as they are in the history, and not as suited as a single commit. The merge (if that is what is being used in this repository) will give a point that all changes can be reverted, and then a choice can still be made to cherry-pick one or more of the three commits.

So, "always rebase and squash" or "always merge" are like most things with absolutes; not always the right thing to do.

ErikSchierboom commented 2 years ago

I hope you are OK with this procedure @kotp and @ErikSchierboom? I ask because it is my first merge here. In the future I will only bother you if I am uncertain or if you say I should do so.

Yes that is fine.

So, "always rebase and squash" or "always merge" are like most things with absolutes; not always the right thing to do.

That is correct, but we do have an Exercism-wide preference for "squash and merge". In fact, for many repos that is the only option.

rainij commented 2 years ago

@kotp I prefer a squash as the default strategy. I like it when the PR gets merged as one atomic commit. Usually the single commits on its own do not make that much sense. And in my opinion the history of main can get clearer if one has one commit for each PR with a good commit-message.

Of course if somebody else does not want to do a squash, in general or for one particular PR, then I am fine with it. I mean it is just my habit to always do squash and so far I had no problems with it and am comfortable with it.

kotp commented 2 years ago

As long as you do it consciously and with the consequences in mind, of course I do not really mind. And we can always tease apart a squashed commit if we need to revert something.

rainij commented 2 years ago

@guygastineau I tested it. Everything is fine.

As I said I will do a "Squash and merge", with a brief summary what you have done.

@kotp I think you are nevertheless fine with the squash? I wait a little longer with my merge to give you a chance to send feedback if you are not.

But would be good to have it in main before the weekend. Currently the redeploy script (in main) overwrites the space-age-testlib, which is no big deal since we are aware of it, but nevertheless not very good. This PR corrects this issue.

kotp commented 2 years ago

Sounds good to me, and yes, not opposed, I just like to defer to the design of the contributor, with their history choices, since they are the ones deep in the material. (So instead of asking me, might ask @guygastineau if his intention was to have one commit.)

guygastineau commented 2 years ago

Yay!

While I do appreciate the richness of history that git can track, I don't mind if you squash these commits.

rainij commented 2 years ago

@kotp I am surprised to see that the PR is already merged. I was just waiting because @kotp expressed some concerns about my proposed merging style and asked to wait for feedback from @guygastineau (which was given 5h ago). Actually I had no doubts that @guygastineau would mind, but I waited nevertheless. Otherwise I would have already merged it by myself after my comment (13h ago):

@guygastineau I tested it. Everything is fine.

I would have used a briefer commit message:

The exercise space-age is using a variation of nearTo that seems more
coherent to us. We changed lib/testlib.sml to use this variation and
redeployed it to all exercises.

I am not sure if the reviewer should do the merge or not. Or if there is some other policy I have to follow. After @ErikSchierboom reply

I hope you are OK with this procedure @kotp and @ErikSchierboom? I ask because it is my first merge here. In the future I will only bother you if I am uncertain or if you say I should do so.

Yes that is fine.

So, "always rebase and squash" or "always merge" are like most things with absolutes; not always the right thing to do.

That is correct, but we do have an Exercism-wide preference for "squash and merge". In fact, for many repos that is the only option.

I thought that as long as the person who merges does a reasonable thing, then this is OK and I thought this question about merging strategy is settled (in a relaxed way). For my next PR review I might do I am a little bit confused now. Should I leave the merge to one of the exercism staff?

ErikSchierboom commented 2 years ago

I am not sure if the reviewer should do the merge or not. Or if there is some other policy I have to follow. After @ErikSchierboom reply

In general, our policy is to have the person submitting the PR do the merge.

I thought that as long as the person who merges does a reasonable thing, then this is OK and I thought this question about merging strategy is settled (in a relaxed way).

It is, but there are disadvantages to a regular merge. I can't seem to find the relevant discussion though.

rainij commented 2 years ago

@ErikSchierboom OK did not know that. I am fine with the author merging the PR (I would not allow it for everybody in my own projects, but I follow exercism policy if this works out well here).

But there is at least a technical problem. Currently not everybody is allowed to hit the merge button, even if the PR is approved by some maintainer or staff. Now that I am maintainer I see that here https://github.com/exercism/sml/pull/208 I could merge it on my own since you approved it. But before in other PRs it was different, github told me that somebody with write-access is needed.

Or am I misunderstanding something?

ErikSchierboom commented 2 years ago

But there is at least a technical problem. Currently not everybody is allowed to hit the merge button, even if the PR is approved by some maintainer or staff. Now that I am maintainer I see that here https://github.com/exercism/sml/pull/208 I could merge it on my own since you approved it. But before in other PRs it was different, github told me that somebody with write-access is needed.

Sorry, I was applying this to this particular use case, where the pull request is submitted by someone with commit permissions to the repo.

There are basically two flows:

  1. A pull request is submitted by someone with commit/merge permissions to the repo
  2. A pull request is submitted by someone without commit/merge permissions to the repo

In both cases, the pull requests need to be approved before merging. After that:

  1. The pull request owner should merge the pull request (they have commit/merge permissions)
  2. A maintainer (who has commit/merge permissions) should merge the pull request
kotp commented 2 years ago

@rainij Yes, since @guygastineau approved the squash, I did the squash. They may have been able to do so, I believe they are in the reviewers list. Since they approved that the individual commits were fine as squashed commit comments, rather than individual commits, I did the squash and merge as indicated.

And now the weekend is here. So several communications to indicate that this was ready to come in, as well as the reviews. So we also made it before this weekend.