exercism / pharo-smalltalk

Exercism exercises in Pharo.
https://exercism.org/tracks/pharo-smalltalk
MIT License
34 stars 28 forks source link

split tools out of exercises repo #105

Open bencoman opened 5 years ago

bencoman commented 5 years ago

I'm having a hard time working with tools and exercises mixed in one repo. Here is the state of the system immediately after loading just the ExercismTools package, just prior to creating the "Exercism" package. I see..

a14-before-adding-exercism-package

After "Add package..." > "Exercism" I now see...

a15-after-adding-exercism-package

My other concern is that its terribly easy for a curious student (of which I expect many) to load the "Exercism" package and get all the answers. I think this would really discourage students from working through the course.

I propose we split ExercismTools package out to https://github.com/exercism/exercism-pharo-tooling. This will...

Leave Exercism and ExercismDev here.

bencoman commented 5 years ago

My preliminary research is that this is not too hard to do... https://confluence.atlassian.com/bitbucket/split-a-repository-in-two-313464964.html https://help.github.com/articles/splitting-a-subfolder-out-into-a-new-repository/ https://www.atlassian.com/blog/git/tear-apart-repository-git-way

samWson commented 5 years ago

Definitely like this suggestion, for the reason of avoiding handing all solutions to students, above all others.

macta commented 5 years ago

Ben are you sure about this? I’ve never had this problem? I developed and checked in all 5 user exercises using the normal tools and they committed without error and with nothing unusual?

Is this a Windows thing (world be odd)

macta commented 5 years ago

I would be against splitting and sub folders - it’s too complicated and leads to lots of confusion.

Keep it simple.

I’m not convinced we actually have a problem.

macta commented 5 years ago

re-reading - you can’t get around students getting solutions from git - this is often discussed in the exercism channels. Let’s leave this to them - I think we should just fit in and work with their approach otherwise we just end up building our own parallel solution and the aim here is to show Pharo can work just like the other tools but be a bit novel.

Equally - loading code into Pharo unconventionally (which exercism drives is to), is going to cause some things like dirty packages etc. I say live with it - the aim is to teach people the language and make them curious - they can learn the other stuff when hooked. We need to make it easy to progress through the exercises and focus on the problems and less about version control etc.

I worry this is all a distraction for now.

Maybe we can do better at some point, but we need to get this working. We actually were in a good place apart from Windows - the OS X/Linus solution with exec was ok.

bencoman commented 5 years ago

I developed and checked in all 5 user exercises using the normal tools and they committed without error and with nothing unusual?

Seems like a different workflow. Yes, if you are "developing" the exercises and have checked out all the whole "Exercism" package, then you'd not see the issue. But it seems like you didn't try my use case. Could you try it and then let me know what your thoughts?

Actually, are you working in an image with the latest Iceberg (per https://github.com/pharo-vcs/iceberg) ?
My example above was based on that. With the default Iceberg in Pharo 6.1 I got a slightly different result that I think is worse. Try this...

  1. Opened fresh Pharo 6.1.
  2. In Playground evaluated...
    
    Metacello new 
    baseline: 'Exercism'; 
    repository: 'github://exercism/pharo:master/dev/src';
    load.
    ExercismDownload exercise: 'hello-world'.

(ExercismManager >> #viewOnExercism:) browse.


2. Made a tools improvement adding a comment to that method.

3. Opened Iceberg to commit the tools change...
Right-clicked on "pharo" repository > Create new branch > "test1"
Right-clicked on "pharo" repository > Synchronize repository
Clicked on <Commit and Push onto test1>.

Is that an reasonable workflow? ...to improve the tools while in the middle of testing the exercises?

4. Reviewed the result... https://github.com/exercism/pharo/compare/master...test1
I only wanted to change the tools. Do you think the other changes might causing confusion for anyone? 
macta commented 5 years ago

Hi Ben - I am using the the newer Iceberg - but I've tried to create a new image and I'm struggling in a hotel room to do it.

One thing I note - you aren't loading the dev baseline above (see the readme in the project). I also wouldn't expect you to download anything with the Exercism commands as the dev baseline brings in all of the exercsies and tools?

macta commented 5 years ago

I am trying to re figure out the magic foo to download the project using iceberg - https://github.com/exercism/pharo/tree/master/dev/src doesn't work for a remote repo. I know you can do it - but can't get the right url (this would make the instructions a bit better so you dont have to use the playground)

bencoman commented 5 years ago

I use "Add > Clone remote repository" and copy the string "git@github.com:exercism/pharo.git" from the big green Clone or download button on github. Then go to Packages and for a student experience just load ``ExercismTools```.

Actually I think the Playground instructions are simpler, and equivalent to command-line use of all other languages. While looking at Iceberg's list of packages, consider that a curious student may load "Exercism" even though its not part of the instructions - because its so simple and just staring them in the face. Then they have the extra required step to unload "Exercism" before they start the course.

macta commented 5 years ago

For students - the command line is the simplest of all and matches the experience with other languages. This is what’s already described in the exercism intro/install pages. I think we should update windows to match - install Pharo by download and unzip (perhaps not via launcher - let’s keep that out of the way). Then run the same commands as OS X/Linux. Far less to go wrong this way. And no unfamiliar playground steps to explain. All this can come later (at least this is what I’ve documented thus far before Windows reared it’s head).

For dev’s - this seems like where you’ve gone off piste, you need to load the correct baseline as described in the readme. However you are correct that it needs updating a bit for iceberg 1.1 (aside: i was trying to use https - something I had to use in the office I was in, but that was a bit rare - so yeah normal ssh is perhaps less steps and what I should have done today when testing it again). There is a dev baseline you have to enter in iceberg (using the playground as documented isn’t ideal as it’s then harder to use iceberg to commit cleanly). If you do it as planned - it works fine.

You also have to follow the generation steps documented in the readme as well.

bencoman commented 5 years ago

I'll tag this on HOLD for the moment. I'm not convinced yet, but I'll try aligning more with the given workflow and re-evaluate.

macta commented 5 years ago

This does remind me, that depending on how we get submit working with relevant meta-data we might not need to shell and run a cli exercism generate to create that data and readme file. The latter file can be created by getting the exercise solution readme and appending our class data.

V2 for sure - but something to remember.

macta commented 5 years ago

I just found a minute to load into a clean image - and I don't see the problems you are seeing @bencoman? This is pharo 6.1 with latest iceberg - and using the "dev" baseline. I do think the readme dev instructions are a bit outdated however - so I will quickly update those to reflect using the newer iceberg (which was only half documented).

image

NOTE: this does expose a small Iceberg issue - as the exercism repo is called pharo (to reflect their conventions) it is a bit confusing seeing pharo in the iceberg project list - I will raise that in the mailing list. You should either be able to rename it, or maybe show the name as exercism/pharo

macta commented 5 years ago

@bencoman I've created a pr #111 to hopefully make it clearer (I think it was a bit hidden before)

macta commented 5 years ago

198 is a more compelling reason to seperate out, however for now a simple IceRepository reset if in user mode is doing the trick

bencoman commented 5 years ago

@macta I just found this ExercismManager class >> welcome ...

ExercismManager class >> welcome
    ...
    "If in usermode, hide the repositories so solutions aren't easily visible with a git compare"
    self isUserMode
        ifTrue: [ 
            IceRepository reset.
            self disableStudentCritics ].

ExercismManager class >> isUserMode
    "Answer true if exercism is loaded in a clean image with no dev tools"

    ^ ((IceRepository registry collect: [ :repo | repo name ])) asArray
        = #('iceberg' 'pharo' 'pharo-smalltalk')
        and: [ (RPackageOrganizer default
                packageNamed: 'ExercismDev'
                ifAbsent: [ nil ]) isNil ]

and to me this smells bad. Now I understand my Iceberg going blank a few times which perturbed me and thought it was a Pharo bug. [Edit: So I'm looking statically at the logic and it seems when isUserMode is true Iceberg is reset, thereafter isUserMode is false so the disableStudentCritics doesn't get run again if it changes in an upgrade. Also, some people may load our Exercism alongside other repos, and clearing those out of Iceberg is not nice.]

An additional reason I'd like to advocate again for separating out the tools is the "filename too long error" I'm getting loading 'dev' baseline for Mocketry on Windows from https://github.com/dionisiydk/Mocketry. I fixed this converting to Tonel here... https://github.com/bencoman/Mocketry and we could reference this in the baseline, but actually I believe its useful for projects to maintain their own forks of code, so I've been thinking we might create an organisation to hold these so the baseline could reference https://github.com/pharo-exercism/Mocketry[1], and the tools could be held next to there[2]. No big rush on [2], but also what are your thoughts on [1]?

macta commented 5 years ago

@bencoman - yes I agree this is a hack to get us over the line (the isUserMode is to only do this in a clean image that has just loaded the default baseline - e.g. you are an Exercism user and not a dev or mentor).

The most compelling reason in my mind is still #198 (as mentioned above) - however a new use case is emerging - we've seen a few times now where users are trying to help us correct exercises and are editing the generated output and not the source exercises. If they are in a seperate repo, it would be much easier to make that distinction - but more compelling is that we could trigger the generation of updates to the exercism repo in a build pipeline (so that its not forgotten). If github we could setup a travis job, or in Gitlab use pipelines (not sure what the latest trend is).

Still its a bit of work - and perhaps we should try and get a few more exercises nailed first? And then tackle this one? It would be nice to have some more harder ones and a few more OO ones. Also the mentoring tools so that more people can help us mark exercises (our queue is getting longer and longer)

bencoman commented 5 years ago

agreed on all points. It can wait for now, but good to know separating tools is a reasonable plan. So any objections to point [1]? I'll create https://github.com/pharo-exercism/Mocketry from my Tonel upgrade. I think keeping it on github will be easier to keep in sync - only single credentials.