Distinguish between path and filepath by kytrinyx · Pull Request #705 · exercism/cli

kytrinyx · 2018-08-11T22:21:43Z

Use a normalized path relative to the exercise directory and using forward slashes when submitting to the server. This is converted to a proper filepath on the file system, using / or \ depending on the operating system.

This builds on top of the pull request in #703 -- too look at just the difference between the two branches, compare no-leading-slash to path-vs-filepath.

Closes #698
Closes #665

Given the path to an exercise directory on the filesystem, determine the root, the track, and the exercise slug.

This normalizes the file sent to the server to use forward slashes.

kytrinyx · 2018-08-12T16:39:19Z

I've rebased this onto master.

nywilken · 2018-08-13T17:56:46Z

Thanks @kytrinyx. I’ll have time to review later today.

nywilken · 2018-08-14T04:09:45Z

workspace/document.go

+// Path is the normalized path.
+// It uses forward slashes regardless of the operating system.
+func (doc Document) Path() string {
+	path := strings.Replace(doc.Filepath, doc.Root, "", 1)


Are these two lines doing the same thing as path, _ := filepath.Rel(doc.Root, doc.Filepath)? Ignoring error right now.

Oh. My. Godness.

I didn't know about filepath.Rel. That's exactly what I need.

nywilken · 2018-08-14T04:16:27Z

workspace/document.go

+	Filepath string
+}
+
+// NewDocument creates a document from a filepath.


Lets add a comment on what is Root and what is Filepath. It seems like Filepath is more of a relative path to the file.

Yepp, good call.

nywilken

@kytrinyx I left a few comments to help clarify a few items. I like the approach and the concept of exercise documents.

I’ll take another look in the am

nywilken · 2018-08-14T04:40:21Z

cmd/submit.go

 			continue
 		}
-		paths = append(paths, file)
+		exercise.Documents = append(exercise.Documents, exercise.NewDocument(file))


Would it be clearer here if it was workspace.NewDocument(exercise.Filepath, file)

I don't feel strongly either way.

nywilken · 2018-08-14T04:42:05Z

workspace/exercise.go

 }
+
+// NewDocument creates a document relative to the exercise.
+func (e Exercise) NewDocument(file string) Document {


Method on this type really feels like it should be a single function or just deleted as workspace.NewDocument does the same thing.

I'll go ahead and make the change--I really don't have a preference one way or the other.

nywilken · 2018-08-14T04:46:28Z

workspace/document.go

+func NewDocument(root, file string) Document {
+	return Document{
+		Root:     root,
+		Filepath: file,


Should this be Filename?

It's a path relative to the root. So for clojure exercises it would be src/anagram.clj

nywilken

@kytrinyx I took another look. We have an issue with the latest change and I think we might be losing functionality. I will confirm the latter once I get behind a machine.

nywilken · 2018-08-15T03:42:04Z

cmd/submit.go

-	for _, path := range paths {
-		file, err := os.Open(path)
+	for _, doc := range exercise.Documents {
+		file, err := os.Open(doc.Filepath)


I haven’t verified this yet. But I think we are losing the ability to submit via full path here.

doc.Filepath is a relative path so if I submit outside of my track calling open here may result in file not exist error. We should add a test TestSubmitWithFullPaths

Good catch! I'll add a test.

nywilken · 2018-08-15T03:42:46Z

cmd/submit.go

-		filename := pieces[len(pieces)-1]
-
-		part, err := writer.CreateFormFile("files[]", filename)
+		part, err := writer.CreateFormFile("files[]", doc.Path())


doc.Path() now returns an error as a second value which now needs to be handled

Dang. Why did this compile?

I guess maybe it didn't but I was sure I had run all the tests...

kytrinyx · 2018-08-19T18:07:42Z

I think I got this sorted out. I've moved the filepath.Rel logic into the NewDocument() function instead of keeping it in the normalized Path() method. @nywilken would you take another look at this?

nywilken

Nicely done! This works as intended when submitting using full and relative paths.

I requested a change on the documentation for NewDocument which has an unwanted behavior if path is relative. It is not an issue for the commands because we are always passing the absolute path. I provided code to give context. If the documentation is clear we may not need to change anything. It really depends on how flexible we want the code to be.

nywilken · 2018-08-20T13:17:34Z

workspace/document.go

+
+// NewDocument creates a document from a relative filepath.
+// The root is typically the root of the exercise, and
+// path is the relative path to the file within the root directory.


Path is no longer a relative path. Path needs to be an absolute path, rooted with root, or filepath.Rel will return an error.

calling filepath.Abs before calling filepath.Rel in this method is an option but it can result in unwanted behavior. https://play.golang.org/p/cLE1R9dsRcH

I think a filepath.HasPrefix(root, path) check before .Rel would catch it.

kytrinyx · 2018-08-20T19:12:35Z

If the documentation is clear we may not need to change anything. It really depends on how flexible we want the code to be.

I actually went back and forth on that a bit, but concluded that I don't want the flexibility until we find that we need it. Every time I get myself in trouble with complexity it's because I thought we needed flexibility that we really didn't need.

I'll fix the doc comment!

nywilken · 2018-08-20T21:57:55Z

I actually went back and forth on that a bit, but concluded that I don't want the flexibility until we find that we need it.

Good call! Comment is good and will save us time in the future if we need to revisit.

nywilken

This is good to Go!

kytrinyx · 2018-08-21T19:27:52Z

Yay, thanks so much for your patience working through this one. I'll cut a release tonight.

kytrinyx added the size/small label Aug 11, 2018

Katrina Owen added 3 commits August 12, 2018 10:39

Create exercise from path to exercise directory

556b340

Given the path to an exercise directory on the filesystem, determine the root, the track, and the exercise slug.

Add documents to Exercise type

49c6c72

Use Exercise type in submit command

0385ab8

This normalizes the file sent to the server to use forward slashes.

kytrinyx force-pushed the path-vs-filepath branch from 0066dfe to 0385ab8 Compare August 12, 2018 16:39

nywilken reviewed Aug 14, 2018

View reviewed changes

Katrina Owen added 2 commits August 14, 2018 17:18

Remove unnecessary indirection on Exercise type

5ed77dd

Clarify doc comment for NewDocument

efe66bd

nywilken reviewed Aug 15, 2018

View reviewed changes

kytrinyx force-pushed the path-vs-filepath branch from ca4f60b to 916aa59 Compare August 19, 2018 14:23

Replace gnarly logic with standard library call

e5a7345

kytrinyx force-pushed the path-vs-filepath branch from 916aa59 to e5a7345 Compare August 19, 2018 18:06

nywilken reviewed Aug 20, 2018

View reviewed changes

Fix incorrect doc comment for NewDocument

60b6ed7

nywilken approved these changes Aug 20, 2018

View reviewed changes

kytrinyx merged commit 78cbd41 into master Aug 21, 2018

kytrinyx deleted the path-vs-filepath branch August 21, 2018 19:27

Uh oh!

Conversation

kytrinyx commented Aug 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kytrinyx commented Aug 12, 2018

Uh oh!

nywilken commented Aug 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nywilken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nywilken left a comment

Choose a reason for hiding this comment

Uh oh!

nywilken Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kytrinyx commented Aug 19, 2018

Uh oh!

nywilken left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kytrinyx commented Aug 20, 2018

Uh oh!

nywilken commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nywilken left a comment

Choose a reason for hiding this comment

Uh oh!

kytrinyx commented Aug 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kytrinyx commented Aug 11, 2018 •

edited

Loading

nywilken Aug 15, 2018 •

edited

Loading

nywilken left a comment •

edited

Loading

nywilken commented Aug 20, 2018 •

edited

Loading