Skip to content
Closed
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cmd/download_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"testing"

"github.com/exercism/cli/config"
ws "github.com/exercism/cli/workspace"
"github.com/spf13/pflag"
"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -167,7 +168,7 @@ func assertDownloadedCorrectFiles(t *testing.T, targetDir, requestor string) {
},
{
desc: "the solution metadata file",
path: filepath.Join(targetDir, "bogus-track", "bogus-exercise", ".solution.json"),
path: filepath.Join(targetDir, "bogus-track", "bogus-exercise", ws.SolutionMetadataFilepath()),
contents: fmt.Sprintf(metadata, requestor),
},
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/prepare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func TestPrepareTrack(t *testing.T) {

expected := []string{
".*[.]md",
"[.]solution[.]json",
"[.]?solution[.]json",
"_spec[.]ext$",
}
track := cliCfg.Tracks["bogus"]
Expand Down
62 changes: 62 additions & 0 deletions cmd/submit_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cmd

import (
"encoding/json"
"io/ioutil"
"net/http"
"net/http/httptest"
Expand Down Expand Up @@ -260,6 +261,50 @@ func TestSubmitOnlyEmptyFile(t *testing.T) {
assert.Regexp(t, "No files found", err.Error())
}

func TestSubmitExerciseWithLegacySolutionMetadataFileAndGetsMigrated(t *testing.T) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about calling this method TestLegacySolutionMetadataMigration?

Copy link
Copy Markdown
Contributor Author

@jdsutherland jdsutherland Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestLegacySolutionMetadataMigration is more readable. The only potential concern I can think of is that the test isn't just testing migration but that a submission is still successful given a legacy solution file - is it worth indicating that in the name?

oldOut := Out
oldErr := Err
Out = ioutil.Discard
Err = ioutil.Discard
defer func() {
Out = oldOut
Err = oldErr
}()
// The fake endpoint will populate this when it receives the call from the command.
submittedFiles := map[string]string{}
ts := fakeSubmitServer(t, submittedFiles)
defer ts.Close()

tmpDir, err := ioutil.TempDir("", "legacy-metadata-file")
assert.NoError(t, err)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These files are not being cleaned up after the tests. A deferred call os.RemoveAll should do the trick.

dir := filepath.Join(tmpDir, "bogus-track", "bogus-exercise")
os.MkdirAll(dir, os.FileMode(0755))
writeFakeLegacySolution(t, dir, "bogus-track", "bogus-exercise")

file := filepath.Join(dir, "file.txt")
err = ioutil.WriteFile(file, []byte("This is a file."), os.FileMode(0755))
assert.NoError(t, err)

v := viper.New()
v.Set("token", "abc123")
v.Set("workspace", tmpDir)
v.Set("apibaseurl", ts.URL)
cfg := config.Configuration{
Persister: config.InMemoryPersister{},
Dir: tmpDir,
UserViperConfig: v,
}
expectedSolutionPathAfterMigration := filepath.Join(dir, workspace.SolutionMetadataFilepath())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to check that this file doesn’t exist prior to executing submit?

@kytrinyx thoughts, Is it too much?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it would help clarify intent and ensure completeness.


err = runSubmit(cfg, pflag.NewFlagSet("fake", pflag.PanicOnError), []string{file})
assert.NoError(t, err)
assert.Equal(t, "This is a file.", submittedFiles[string(os.PathSeparator)+"file.txt"])

_, err = os.Stat(expectedSolutionPathAfterMigration)
assert.NoError(t, err)
}

func TestSubmitFilesFromDifferentSolutions(t *testing.T) {
tmpDir, err := ioutil.TempDir("", "dir-1-submit")
assert.NoError(t, err)
Expand Down Expand Up @@ -331,3 +376,20 @@ func writeFakeSolution(t *testing.T, dir, trackID, exerciseSlug string) {
err := solution.Write(dir)
assert.NoError(t, err)
}

func writeFakeLegacySolution(t *testing.T, dir, trackID, exerciseSlug string) {
solution := &workspace.Solution{
ID: "bogus-solution-uuid",
Track: trackID,
Exercise: exerciseSlug,
URL: "http://example.com/bogus-url",
IsRequester: true,
}
legacySolutionFilePath := filepath.Join(dir, ".solution.json")

b, err := json.Marshal(solution)
assert.NoError(t, err)

err = ioutil.WriteFile(legacySolutionFilePath, b, os.FileMode(0600))
assert.NoError(t, err)
}
2 changes: 1 addition & 1 deletion config/cli_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func TestCLIConfigSetDefaults(t *testing.T) {
Tracks: map[string]*Track{
"bogus": {
ID: "bogus",
IgnorePatterns: []string{"[.]solution[.]json", "_spec[.]ext$"},
IgnorePatterns: []string{"[.]?solution[.]json", "_spec[.]ext$"},
},
},
}
Expand Down
2 changes: 1 addition & 1 deletion config/track.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (

var defaultIgnorePatterns = []string{
".*[.]md",
"[.]solution[.]json",
"[.]?solution[.]json",
}

// Track holds the CLI-related settings for a track.
Expand Down
58 changes: 44 additions & 14 deletions workspace/solution.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@ import (
"path/filepath"
"strings"
"time"

"github.com/exercism/cli/visibility"
)

const solutionFilename = ".solution.json"
const ignoreSubdir = ".exercism"
const solutionFilename = "solution.json"

// Solution contains metadata about a user's solution.
type Solution struct {
Expand All @@ -29,7 +28,7 @@ type Solution struct {

// NewSolution reads solution metadata from a file in the given directory.
func NewSolution(dir string) (*Solution, error) {
path := filepath.Join(dir, solutionFilename)
path := filepath.Join(dir, SolutionMetadataFilepath())
b, err := ioutil.ReadFile(path)
if err != nil {
return &Solution{}, err
Expand Down Expand Up @@ -61,22 +60,19 @@ func (s *Solution) String() string {
}

// Write stores solution metadata to a file.
func (s *Solution) Write(dir string) error {
func (s *Solution) Write(path string) error {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion this variable should remain as dir in a previous version you were generating the path, but still using dir to concat filenames.

I think what you have now is good. I just recommend that you keep dir for the exercise directory and use path for the SolutionMetadataPath

b, err := json.Marshal(s)
if err != nil {
return err
}

path := filepath.Join(dir, solutionFilename)

// Hack because ioutil.WriteFile fails on hidden files
visibility.ShowFile(path)

if err := ioutil.WriteFile(path, b, os.FileMode(0600)); err != nil {
if err = createIgnoreSubdir(path); err != nil {
return err
}
s.Dir = dir
return visibility.HideFile(path)
if err = ioutil.WriteFile(filepath.Join(path, SolutionMetadataFilepath()), b, os.FileMode(0600)); err != nil {
return err
}
s.Dir = path
return err
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return nil is a bit more clear that no error is being returned

}

// PathToParent is the relative path from the workspace to the parent dir.
Expand All @@ -87,3 +83,37 @@ func (s *Solution) PathToParent() string {
}
return filepath.Join(dir, s.Track)
}

// SolutionMetadataFilepath is the path of the solution metadata file relative to the workspace.
func SolutionMetadataFilepath() string {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how this works. Each exercise has its own metadata file, so this is relative to a subdirectory within the workspace.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mistakenly used 'workspace' when I meant 'exercise' there.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that makes sense. Thanks!

return filepath.Join(ignoreSubdir, solutionFilename)
}

func createIgnoreSubdir(path string) error {
path = filepath.Join(path, ignoreSubdir)
if _, err := os.Stat(path); os.IsNotExist(err) {
if err := os.Mkdir(path, os.FileMode(0755)); err != nil {
return err
}
}
return nil
}

func migrateLegacySolutionFile(legacySolutionPath string, solutionPath string) error {
if _, err := os.Lstat(legacySolutionPath); err != nil {
return err
}
if err := createIgnoreSubdir(filepath.Dir(legacySolutionPath)); err != nil {
return err
}
if _, err := os.Lstat(solutionPath); err != nil {
if err := os.Rename(legacySolutionPath, solutionPath); err != nil {
return err
}
fmt.Fprintf(os.Stderr, "\nMigrated solution metadata to %s\n", solutionPath)
} else {
// TODO: decide how to handle case where both legacy and modern metadata files exist
fmt.Fprintf(os.Stderr, "\nAttempted to migrate solution metadata to %s but file already exists\n", solutionPath)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the new metadata takes precendence which leads into my next comment about workspace,checkSolutionFile. It currently checks for a legacy metdata first, but I think we should check for the new path first and only check for legacy if it doesn’t exist.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. The most common path should be checked first.

}
return nil
}
14 changes: 13 additions & 1 deletion workspace/workspace.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,21 @@ func (ws Workspace) SolutionDir(s string) (string, error) {
if _, err := os.Lstat(path); os.IsNotExist(err) {
return "", err
}
if _, err := os.Lstat(filepath.Join(path, solutionFilename)); err == nil {
if err := checkSolutionFile(path); err == nil {
return path, nil
}
path = filepath.Dir(path)
}
}

func checkSolutionFile(path string) error {
legacySolutionPath := filepath.Join(path, ".solution.json")
solutionPath := filepath.Join(path, SolutionMetadataFilepath())

if _, err := os.Lstat(legacySolutionPath); err == nil {
return migrateLegacySolutionFile(legacySolutionPath, solutionPath)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don’t rename and instead warn the use that a deprecated solution files exists would it make the migration easier. @kytrinyx @NobbZ thoughts on leaving the file or renaming it for the user.

I recommended renaming but I’m not sure how users will feel about the CLI renaming

} else if _, err := os.Lstat(solutionPath); err != nil {
return err
}
return nil
}