Issue 96: Submit exercises via HTTP patch method #110

samWson · 2018-09-26T08:50:18Z

I broadly understand what this thing needs to do now. I won't be able to spend anytime on this tomorrow so I'm placing this pull request here to make sure I'm on the right track. Hopefully I'll be able to get back to it this weekend.

This is definitely a work in progress. A few more helper methods will probably be made, plus getting rid of some hard coded values.

I used the Acronym exercise as an example, which means I could not test submitting because I have not downloaded the exercise metadata, which I can't do because I haven't unlocked the exercise ¯_(ツ)_/¯ .

I've left a few comments in the source code with my thoughts and a few problems I haven't yet solved. Let me know what you think.

I'm also irritates me that I don't have any tests of the public interface. For lack of a good mocking framework it will take a bit of work to mock Exercisms API, and the OS file system.

samWson · 2018-09-26T08:52:21Z

dev/src/ExercismTools/ExercismSubmit.class.st

+ExercismSubmit >> buildRequest [
+	| solutionEntity solutionPart multiPartFormDataEntity solutionId |
+	solutionEntity := ZnByteArrayEntity bytes: exerciseContents.
+	solutionPart := ZnMimePart exercismFieldName: 'files[]' fileName: 'Acronym.class.st' entity: solutionEntity.


Hard coded file name. There might be multiple files for a solution i.e. the class side and instance side of a class. I'm not that familiar with using Smalltalk on file systems. This is a problem I haven't solved yet.

samWson · 2018-09-26T08:57:42Z

dev/src/ExercismTools/ExercismSubmit.class.st

+	| path |
+	exercise := aStringExercise.
+
+	path := (Path from: (aStringExercise asKebabCase)) / '.pharo' / aStringExercise capitalized , 'class.st'.


Again solution hard coded here: class.st. Seems my number one problem is getting data from all the possible files a exercise could have. I figure for an exercise we will need to grab all files in the directory ending with .st and make a mime part for each, excluding the test class and the package file. I don't have much experience walking file systems so it's a new problem to me.

Also this code assumes that Pharo is being run from the Exercism/pharo directory.

macta · 2018-09-26T09:31:42Z

I havent had a chance to look at your code (family duties are in the way),but from your comments I worry you may have gone astray... aren’t we now divorced from the file system? You should be able to iterate over the classes in an exercise sub-pkg/tag and generate the source for each and attach it as a fake “file” (no real file system should be needed).

possibly I’m missing something - but that’s what went through my head as a soln. (Just as retrieve doesn’t get any real file either).

There is also some metadata stuff we need to do as well - by again it should be file-less

bencoman · 2018-09-26T13:55:28Z

which means I could not test submitting because I have not downloaded the exercise metadata, which I can't do because I haven't unlocked the exercise ¯_(ツ)_/¯ .

Leave the track and then rejoin in independent mode, then all exercises are unlocked. During my testing I've done that multiple times.

samWson · 2018-09-28T08:37:20Z

aren’t we now divorced from the file system?

@macta I did have my intent wrong with using the filesystem. That helps a lot as that was my number one headache.

You should be able to iterate over the classes in an exercise sub-pkg/tag and generate the source for each and attach it as a fake “file” (no real file system should be needed).

The idea is to have a string of source code in Tonel format for each class excluding the test class, correct? We were originally doing submitting of the exercises from ExercismManager>>#submitToExercism: I had intended to replace the OSProcess code with the ExercismSubmit command in there. If we continue down that path I can remove the ExTonelWriter so we are not writing to the file system anymore.

On generating the source for each exercise 'file': I figure the ExercismSubmit command should just get the exercise name as an argument, and from that the command generates the 'files', assembles the HTTP client, and submits the exercise. Let me know if that is what we want, then I can get back to it this weekend.

samWson · 2018-09-30T05:03:09Z

I'm very stuck on this at the moment. I've been trying to get the source code of a exercise in Tonel format before adding it to the submit client as Mime parts (no file system involved, just in the image memory).

My main problem is making a TonelWriter. Creating instances seems to be dependent on a source directory. I was hoping in some fashion to create it just using a class or its RPackageTag then just get the contents of the writer as a string:

"Using exercise acronym as an example."
"Trying to get the source code in Tonel format as a string."
packageTag := (RPackageOrganizer default packageNamed: 'Exercism') classTagForClass: Acronym.

"Problem here: a working copy (.mcz file) is expected in the pharo-local\package-cache\"
version := MCVersion package: packageTag. 

"Won't work because of the above issue."
TonelWriter fileOut: version on: (String new writeStream).

bencoman · 2018-09-30T19:08:21Z

Having a poke at this myself, I feel your pain. Tonel doesn't look written to be independent of filesystem.
Not a full answer, but I notice TonelWriter-class >> fileOut:on: calls...

TonelWriter >> writeVersion: aVersion
	self writeSnapshot: aVersion snapshot

so better way of getting a snapshot could be...

packageTag := (RPackageOrganizer default packageNamed: 'Shout') classTagForClass: SHRange.
snapshot := packageTag snapshot .

but then writeSnapshot: calls...
writePackage:, writeClass:, writeMethodExtensions -->writeExtensionMethods:className:
all of which do...

    self fileUtils 
        writeStreamFor:

which presumes a filesystem and is no use for us to directly get Strings. At which point I see two options:

Use a memory file system like...

	| writer mem nl |
	mem := FileSystem memory root.
	writer := TonelWriter on: mem.
	writer writeSnapshot: self mockSnapshot.

Refactor Tonel methods as our-own-extension** to return a Dictionary of Strings. The top half of writeMethodExtensions already does this, so I think this is feasible. As a first pass I'd ignore the writePackage: and writeMethodExtensions

** Separately we can submit a PR for this to Tonel.

bencoman · 2018-09-30T19:32:50Z

I've been playing around. No time for a PR at 3:30am, but perhaps something like...

TonelWriter >> writeClass: aClassDefinition
	[ 
		self fileUtils 
			writeStreamFor: (self fileNameFor: aClassDefinition) 
			in: self packageDir 
			do: [ :aStream | self writeClass: aClassDefinition on: aStream  ] 
	]
	on: TonelShouldIgnore
	do: [ :e | self logCr: 'ignoring: ', aClassDefinition asString ]

TonelWriter >> writeClass: aClassDefinition on: aStream "extracted from above"
	self writeClassDefinition: aClassDefinition on: aStream.
	self writeClassSideMethodDefinitions: aClassDefinition on: aStream.
	self writeInstanceSideMethodDefinitions: aClassDefinition on: aStream

TonelWriter>>mappedSnapshot: aSnapshot
	"extracted from #writeSnapshot:"
	|tonelMap|
	snapshot := aSnapshot.
	tonelMap := Dictionary new.
	(snapshot definitions select: #isClassDefinition)
		do: [ :classdef |  |tonelStream|
			tonelStream := WriteStream on: String new.
	 		self writeClass: classdef on: tonelStream.
	  		tonelMap at: classdef className put: tonelStream contents ].
	^tonelMap

Playground...
packageTag := (RPackageOrganizer default packageNamed: 'Shout') classTagForClass: SHRange.
snapshot := packageTag snapshot .
tonelMap := TonelWriter new mappedSnapshot: snapshot.

TonelWriterTest remains green.

samWson · 2018-10-01T07:07:07Z

@bencoman thanks for digging into that. I can understand most of it. It gets a good result.

We could adopt the methods above as extension methods. We also already have a ExTonelWriter so we could override some of the behavior of TonelWriter for our use case. I'm in favor of the former since all the tests are still green. If a PR gets accepted its even better for us.

macta · 2018-10-02T03:11:17Z

Nice job moving this forward guys - it shouldn’t have been quite this hard, but the Tonel classes are still quite 1.0 and this usecase probably helps shape them.

Sadly my laptop has died in my travels so I’m even less able to help at the moment - maybe in Sydney I might be able to get it fixed in a few weeks. Anyway, keep pushing.

bencoman · 2018-10-02T16:45:06Z

I'll have a PR for our own extensions shortly.

This change allows submitting exercises without needing to read/write files to the file system. It assumes a TonelWriter capable of using an in memory file system (not included in this change).

samWson · 2018-10-07T07:42:55Z

Just a quick update. I have another commit to push using the changes to the TonelWriter shown above, though the TonelWriter changes are not a part of the commit. I'm having trouble pushing things using Git on Windows at the moment. Hopefully I'll get them sorted out by tomorrow.

samWson · 2018-10-08T06:59:53Z

Here is the latest I have on this. Definitely not finished at this stage, but this should be enough for you to see weather I am on the right track or not.

@bencoman this change makes use of the changes to TonelWriter you made above, although they are not a part of the commit I made here. One thing I haven't accounted for is iterations of solutions but I think you already posted an example of how to do that on the issue earlier.

There is a random merge commit in there. I find Iceberg far less clear to use compared to Git on the command line, and I ended up making a commit that removed the changes I wanted instead of adding them.

I'll leave some comments on my thoughts in the code changes for you to see. Still more work to do but hopefully this should be closer. I think I have the major steps in place but let me know if I've left anything out. Other than handling iterations on solutions, I don't think I've left out anything else.

samWson · 2018-10-08T07:01:14Z

dev/src/ExercismTools/ExercismManager.class.st

-	cmd := 'exercism submit ' , filesToSubmit.
-	result := PipeableOSProcess waitForCommand: cmd.
-	result succeeded ifFalse: [ self error: 'Unable to submit solution, CLI reported ', result outputAndError printString ].
+	ExercismSubmit exercise: exerciseName.


I've placed this here as it seems like the easiest way to plug in to what we already have. I don't know if we intend to keep using the ExercismManager.

dev/src/ExercismTools/ExercismSubmit.class.st

samWson · 2018-10-08T07:05:35Z

dev/src/ExercismTools/ExercismSubmit.class.st

+{ #category : #internal }
+ExercismSubmit >> buildRequest [
+	| solutionEntity solutionPart multiPartFormDataEntity solutionId |
+	"Assumption: There is only one entity to make which is from the class with the same name as the exercise."


I've made an assumption as per the comment. Handling a single class with the same name as the exercise might be enough for minimum viable product. I assume we don't need to submit the test class. If needs be I don't think it is too much trouble to handle more than one class either as a part of this change or later on.

samWson · 2018-10-19T06:24:57Z

@macta @bencoman I hope you two are doing well. It's been a while and I had hoped either of you had managed to review this pull request.

I've been looking at how different iterations of exercise submissions are handled as it is the only thing I'm not sure of for this solution at the moment. Looking around at the Rust and Ruby exercises on my computer didn't give me any clues other than the fact that it doesn't look like iteration tracking is done on the users computer.

In any case I think we are finally getting close to having the whole user cycle of download, work, and submit complete. I'm keen to keep pushing this project along. Give me any feed back if you can. I'll keep making progress where possible. I might merge this soon anyway to keep things moving.

macta · 2018-10-19T11:30:33Z

Sorry, I’ll try and have a look tomorrow night. Managed to get my computer fixed in Sydney, so I can better look at code (using an iPhone is too hard to keep the context in my head)

…ow working.

bencoman · 2018-10-21T14:39:14Z

@samWson Sorry for the delay in reviewing this. I've started a new contract that is stretching my capabilities (rerouting a 66kV transmission line). Managed to find time this weekend to have a good crack at it and got it working. I pushed the above update.

First I need to correct some of my own previous work...
Running your code I bumped into exercismPackage ensureProperties having
duplicate entries for "a RPackageTag(HellowWorld)" caused by removing the HelloWorld package
then re-running ExercismDownload exercise: 'hello-world'. So rather than being tricky and piggy-backing Pharo infrastructure, I added SolutionData to ExercismManager as a Dictionary.

The CLI submit semantics are...
C:> exercism submit -h
Call the command with the >>>list of files<<< you want to submit.

So even if most exercises so far only need one class, I think in #buildRequest your assumption "There is only one entity to make which is from the class with the same name as the exercise" is too narrow. Our "classes" are more equivalent to "files" and an "exercise" relates more to our "packages". I've reworked it to allow submitting single & multiple classes as well as a whole exercise/package by name.

I found...
ApiPath := '/v1/solutions/latest' was not suitable for SUBMIT since it requires the "/latest" stripped off.
(See the PATCH line under "Now Submitting..." heading far above.)
So I changed that to ApiPath := '/v1/solutions' and modified ExercismDownload to match.
IMPORTANT: Evaluate ExercismCommand reset before running #testSubmitHelloWorld.

bencoman · 2018-10-21T14:41:34Z

dev/src/ExercismTools/ExercismCommand.class.st

@@ -71,7 +71,7 @@ ExercismCommand class >> configureToken [
 ExercismCommand class >> configureToken: your_CLI_token [
    "Get your_CLI_token at https://exercism.io/my/settings"
    ApiToken := your_CLI_token.
-    ApiPath := '/v1/solutions/latest'.
+    ApiPath := '/v1/solutions'.


Needed to strip "/latest" to work with SUBMIT.

bencoman · 2018-10-21T14:42:53Z

dev/src/ExercismTools/ExercismDownload.class.st

-					exercismPackage ensureProperties at: tag put: solution.
-				].
-			].
+         	parser document do: [:def | def load].


Solution data now stored in ExercismManager

bencoman · 2018-10-21T14:44:23Z

dev/src/ExercismTools/ExercismSubmit.class.st

-		packageNamed: 'Exercism' 
-		ifAbsent: [ self error: 'No exercises downloaded.' ].
-	classTag := exercismPackage classTagForClass: aClass.  
-	^ exercismPackage ensureProperties at: classTag.


Solution data moved to ExercismManager.

bencoman · 2018-10-21T14:56:10Z

dev/src/ExercismTools/ExercismSubmit.class.st

-	exercismPackage ensureProperties 
-		detect: [ :solutionData | ((solutionData at: 'exercise') at: 'id') = exerciseString ] 
-		ifFound: [ :solutionData | ^ solutionData ] 
-		ifNone: [ self error: 'Exercise ''' , exerciseString , ''' not downloaded.'].


No longer required. Solution data moved to ExercismManager.

bencoman · 2018-10-21T14:57:45Z

dev/src/ExercismTools/ExercismSubmit.class.st

+ExercismSubmit class >> exercise: exerciseId [
+	"Submit whole package-tag specified by exerciseId."
+	"By default, don't submit TestCases with solution (if needed use #classes: direct)"
+	|rootPackage packageTag solutionClasses|


@samWson I changed your semantics here. exerciseId to represent a whole package-tag rather than a single class. Added #class: for submitting single classes.

bencoman · 2018-10-21T14:59:22Z

dev/src/ExercismTools/ExercismSubmit.class.st


+	httpclient 
+		url: ApiPath , '/' , solutionId;
+		entity: multiPartFormDataEntity


Removed '/latest' from ApiPath to make this work here.

bencoman · 2018-10-21T15:00:12Z

dev/src/ExercismTools/ExercismSubmitTest.class.st

-	|solutionData|
-	ExercismDownload exercise: 'hello-world'.
-	solutionData := ExercismSubmit solutionDataForClass: HelloWorld.
-	self assert: ((solutionData at: 'exercise') at: 'id') equals: 'hello-world'.


No longer required. Solution data moved to ExercismManager.

bencoman · 2018-10-21T15:03:40Z

dev/src/ExercismTools/String.extension.st

+{ #category : #'*ExercismTools' }
+String >> kebabAsCamelCase [
+	"Answer a String that converts the CamelCase input to camel-case kebab output 
+	used by exercism"


Whoops, copy-paste error with the comment, should be...

Suggested change

used by exercism"

"Answer a new String converted from Exercism's kebab-case to CamelCase."

samWson · 2018-10-23T07:23:11Z

@bencoman thanks for the help. I see in the commit message you say that ExercismSubmit is now working, so I take it that you have tested this? I'm having trouble submitting or downloading anything so I'm about to try again with a clean image to be sure.

Otherwise I think the only thing left to do is to incorporate the new submit code into the Nautilus menu when you right click on an exercise.

bencoman · 2018-10-23T12:57:30Z

On Tue, 23 Oct 2018 at 15:23, Samuel ***@***.***> wrote: @bencoman <https://github.com/bencoman> thanks for the help. I see in the commit message you say that ExercismSubmit is now working, so I take it that you have tested this?

Yes, I tested in a fresh Pharo 6.1 image. Note it wasn't a completely fresh download. A couple of months ago I downloaded a new Pharo 6.1 image, updated only Iceberg and archived that as my base for Exercism dev. I'm having trouble submitting or downloading anything so I'm about to try

again with a clean image to be sure.

You don't mention whether you did ```ExercismCommand reset``` ? Without that, ```ApiPath``` variable is not updated to the required value, so both both downloading and submitting will be broken.

Otherwise I think the only thing left to do is to incorporate the new submit code into the Nautilus menu when you right click on an exercise.

Once you observe it working, lets merge this PR. Then you can add the GUI part if you like. I am caught up on other things for a few days.

samWson · 2018-10-24T07:44:25Z

@bencoman thanks. It works now. I'm happy to merge this thing at long last. I'll get started on new issue for adding the new submit code to the GUI part.

macta · 2018-11-06T04:56:21Z

@samWson sorry its been just too difficult to contribute while on the road. I did however try downloading the latest image a few days ago expecting to see it use the work you guys have been forging on with - and it still seemed to be using the OS shell stuff? Possibly user error - but master does have this stuff for both pull and submit right? I did see @bencoman 's improved token prompt but when submitting a solution it seemed to be using os shell. I wanted to confirm its all supposed to be in there, and I'll try again when I get a moment.

bencoman · 2018-11-06T05:21:03Z

I don't think the direct-submit functionality has made it through to the GUI yet. Try the (minimal) unit test.

samWson · 2018-11-06T06:17:16Z

@macta we are almost there. Replacing the original code at the GUI is the last bit for this issue. It doesn't look like much work at all, however that's when I ran into my latest problem with Tonel.

@samWson sorry its been just too difficult to contribute while on the road.

That's OK. I don't mind real life taking priority over this project any day. Have a good holiday. I'm happy to keep working on the code as I have time available, and I've always got the Pharo Discord available if I can't get help here :)

Issue 96: Submit exercises via HTTP patch method

b6c757d

samWson added the review code review required label Sep 26, 2018

samWson requested review from bencoman and macta September 26, 2018 08:50

samWson commented Sep 26, 2018

View reviewed changes

samWson added 2 commits October 7, 2018 20:27

Issue 96: Submit exercises via HTTP patch method

33d4291

This change allows submitting exercises without needing to read/write files to the file system. It assumes a TonelWriter capable of using an in memory file system (not included in this change).

Merge with b6c757d

1b48521

samWson commented Oct 8, 2018

View reviewed changes

dev/src/ExercismTools/ExercismSubmit.class.st Show resolved Hide resolved

samWson commented Oct 8, 2018

View reviewed changes

Issue 96: Move SolutionData to ExercismManager; Fix ApiPath; Submit n…

227172f

…ow working.

bencoman reviewed Oct 21, 2018

View reviewed changes

samWson changed the title ~~WIP: Issue 96: Submit exercises via HTTP patch method~~ Issue 96: Submit exercises via HTTP patch method Oct 24, 2018

samWson merged commit ae8611a into master Oct 24, 2018

samWson mentioned this pull request Nov 3, 2018

Submitting solutions direct from Pharo #96

Closed

This was referenced Nov 17, 2018

Testing macOS from scratch, Error: This URL was not recognised #113

Closed

Test user experience on Windows 10 #31

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 96: Submit exercises via HTTP patch method #110

Issue 96: Submit exercises via HTTP patch method #110

samWson commented Sep 26, 2018

samWson Sep 26, 2018

samWson Sep 26, 2018 •

edited

Loading

macta commented Sep 26, 2018 •

edited

Loading

bencoman commented Sep 26, 2018 via email

samWson commented Sep 28, 2018

samWson commented Sep 30, 2018

bencoman commented Sep 30, 2018

bencoman commented Sep 30, 2018 •

edited

Loading

samWson commented Oct 1, 2018

macta commented Oct 2, 2018

bencoman commented Oct 2, 2018 via email

samWson commented Oct 7, 2018

samWson commented Oct 8, 2018 •

edited

Loading

samWson Oct 8, 2018

samWson Oct 8, 2018

samWson commented Oct 19, 2018 •

edited

Loading

macta commented Oct 19, 2018

bencoman commented Oct 21, 2018 •

edited

Loading

bencoman Oct 21, 2018

bencoman Oct 21, 2018

bencoman Oct 21, 2018

bencoman Oct 21, 2018

bencoman Oct 21, 2018 •

edited

Loading

bencoman Oct 21, 2018

bencoman Oct 21, 2018

bencoman Oct 21, 2018

samWson commented Oct 23, 2018

bencoman commented Oct 23, 2018 via email

samWson commented Oct 24, 2018

macta commented Nov 6, 2018

bencoman commented Nov 6, 2018 via email

samWson commented Nov 6, 2018

	used by exercism"
	"Answer a new String converted from Exercism's kebab-case to CamelCase."

Issue 96: Submit exercises via HTTP patch method #110

Issue 96: Submit exercises via HTTP patch method #110

Conversation

samWson commented Sep 26, 2018

Choose a reason for hiding this comment

samWson Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

macta commented Sep 26, 2018 • edited Loading

bencoman commented Sep 26, 2018 via email

samWson commented Sep 28, 2018

samWson commented Sep 30, 2018

bencoman commented Sep 30, 2018

bencoman commented Sep 30, 2018 • edited Loading

samWson commented Oct 1, 2018

macta commented Oct 2, 2018

bencoman commented Oct 2, 2018 via email

samWson commented Oct 7, 2018

samWson commented Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samWson commented Oct 19, 2018 • edited Loading

macta commented Oct 19, 2018

bencoman commented Oct 21, 2018 • 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

bencoman Oct 21, 2018 • 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

samWson commented Oct 23, 2018

bencoman commented Oct 23, 2018 via email

samWson commented Oct 24, 2018

macta commented Nov 6, 2018

bencoman commented Nov 6, 2018 via email

samWson commented Nov 6, 2018

samWson Sep 26, 2018 •

edited

Loading

macta commented Sep 26, 2018 •

edited

Loading

bencoman commented Sep 30, 2018 •

edited

Loading

samWson commented Oct 8, 2018 •

edited

Loading

samWson commented Oct 19, 2018 •

edited

Loading

bencoman commented Oct 21, 2018 •

edited

Loading

bencoman Oct 21, 2018 •

edited

Loading