Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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 @@ -12,6 +12,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 @@ -148,7 +149,7 @@ func TestDownload(t *testing.T) {
metadata = fmt.Sprintf(metadata, tc.requestor)
metadata = compact(t, metadata)

path := filepath.Join(targetDir, "bogus-track", "bogus-exercise", ".solution.json")
path := filepath.Join(targetDir, "bogus-track", "bogus-exercise", ws.SolutionMetadataFilepath())
b, err := ioutil.ReadFile(path)
assert.NoError(t, err)
assert.Equal(t, metadata, string(b), "the solution metadata file")
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
65 changes: 65 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,53 @@ func TestSubmitOnlyEmptyFile(t *testing.T) {
assert.Regexp(t, "No files found", err.Error())
}

func TestLegacySolutionMetadataMigration(t *testing.T) {
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")
defer os.RemoveAll(tmpDir)
assert.NoError(t, err)

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 = os.Stat(expectedSolutionPathAfterMigration)
assert.Error(t, err)

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 @@ -375,3 +423,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
54 changes: 42 additions & 12 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 @@ -66,17 +65,14 @@ func (s *Solution) Write(dir string) error {
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(dir); err != nil {
return err
}
if err = ioutil.WriteFile(filepath.Join(dir, SolutionMetadataFilepath()), b, os.FileMode(0600)); err != nil {
return err
}
s.Dir = dir
return visibility.HideFile(path)
return nil
}

// 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
}
15 changes: 14 additions & 1 deletion workspace/workspace.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,22 @@ 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())

var err error
if _, err = os.Lstat(solutionPath); err == nil {
return nil
} else if _, err2 := os.Lstat(legacySolutionPath); err2 == nil {
return migrateLegacySolutionFile(legacySolutionPath, solutionPath)
}
return err
}