mirror of
https://github.com/jesseduffield/lazygit.git
synced 2025-05-11 12:25:47 +02:00
Cleanup: return didChange bool from computeMigratedConfig
It's a bit silly to find out by string comparison whether computeMigratedConfig did something, when it knows this already and can just return the information. This doesn't make a huge difference to the production code; the string comparison isn't very expensive, so this isn't a big deal. However, it makes the tests clearer; we don't have to bother specifying an expected output string if the didChange flag is false, and in particular we can get rid of the ugly "This test intentionally uses non-standard indentation" bit in one of the tests.
This commit is contained in:
Stefan Haller
parent
b1f8aa6c9e
commit
7c69533aa3
2 changed files with 69 additions and 57 deletions
|
|
@ -220,13 +220,13 @@ func loadUserConfig(configFiles []*ConfigFile, base *UserConfig) (*UserConfig, e
|
|||
// from one container to another, or changing the type of a key (e.g. from bool
|
||||
// to an enum).
|
||||
func migrateUserConfig(path string, content []byte) ([]byte, error) {
|
||||
changedContent, err := computeMigratedConfig(path, content)
|
||||
changedContent, didChange, err := computeMigratedConfig(path, content)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
// Nothing to do if config didn't change
|
||||
if string(changedContent) == string(content) {
|
||||
if !didChange {
|
||||
return content, nil
|
||||
}
|
||||
|
||||
|
|
@ -240,17 +240,17 @@ func migrateUserConfig(path string, content []byte) ([]byte, error) {
|
|||
}
|
||||
|
||||
// A pure function helper for testing purposes
|
||||
func computeMigratedConfig(path string, content []byte) ([]byte, error) {
|
||||
func computeMigratedConfig(path string, content []byte) ([]byte, bool, error) {
|
||||
var err error
|
||||
var rootNode yaml.Node
|
||||
err = yaml.Unmarshal(content, &rootNode)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("failed to parse YAML: %w", err)
|
||||
return nil, false, fmt.Errorf("failed to parse YAML: %w", err)
|
||||
}
|
||||
var originalCopy yaml.Node
|
||||
err = yaml.Unmarshal(content, &originalCopy)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("failed to parse YAML, but only the second time!?!? How did that happen: %w", err)
|
||||
return nil, false, fmt.Errorf("failed to parse YAML, but only the second time!?!? How did that happen: %w", err)
|
||||
}
|
||||
|
||||
pathsToReplace := []struct {
|
||||
|
|
@ -265,46 +265,46 @@ func computeMigratedConfig(path string, content []byte) ([]byte, error) {
|
|||
for _, pathToReplace := range pathsToReplace {
|
||||
err := yaml_utils.RenameYamlKey(&rootNode, pathToReplace.oldPath, pathToReplace.newName)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("Couldn't migrate config file at `%s` for key %s: %s", path, strings.Join(pathToReplace.oldPath, "."), err)
|
||||
return nil, false, fmt.Errorf("Couldn't migrate config file at `%s` for key %s: %s", path, strings.Join(pathToReplace.oldPath, "."), err)
|
||||
}
|
||||
}
|
||||
|
||||
err = changeNullKeybindingsToDisabled(&rootNode)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
|
||||
return nil, false, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
|
||||
}
|
||||
|
||||
err = changeElementToSequence(&rootNode, []string{"git", "commitPrefix"})
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
|
||||
return nil, false, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
|
||||
}
|
||||
|
||||
err = changeCommitPrefixesMap(&rootNode)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
|
||||
return nil, false, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
|
||||
}
|
||||
|
||||
err = changeCustomCommandStreamAndOutputToOutputEnum(&rootNode)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
|
||||
return nil, false, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
|
||||
}
|
||||
|
||||
err = migrateAllBranchesLogCmd(&rootNode)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
|
||||
return nil, false, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
|
||||
}
|
||||
|
||||
// Add more migrations here...
|
||||
|
||||
if reflect.DeepEqual(rootNode, originalCopy) {
|
||||
return content, nil
|
||||
return nil, false, nil
|
||||
}
|
||||
|
||||
newContent, err := yaml_utils.YamlMarshal(&rootNode)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("Failed to remarsal!\n %w", err)
|
||||
return nil, false, fmt.Errorf("Failed to remarsal!\n %w", err)
|
||||
}
|
||||
return newContent, nil
|
||||
return newContent, true, nil
|
||||
}
|
||||
|
||||
func changeNullKeybindingsToDisabled(rootNode *yaml.Node) error {
|
||||
|
|
|
|||
|
|
@ -8,14 +8,15 @@ import (
|
|||
|
||||
func TestCommitPrefixMigrations(t *testing.T) {
|
||||
scenarios := []struct {
|
||||
name string
|
||||
input string
|
||||
expected string
|
||||
name string
|
||||
input string
|
||||
expected string
|
||||
expectedDidChange bool
|
||||
}{
|
||||
{
|
||||
name: "Empty String",
|
||||
input: "",
|
||||
expected: "",
|
||||
name: "Empty String",
|
||||
input: "",
|
||||
expectedDidChange: false,
|
||||
},
|
||||
{
|
||||
name: "Single CommitPrefix Rename",
|
||||
|
|
@ -29,6 +30,7 @@ func TestCommitPrefixMigrations(t *testing.T) {
|
|||
- pattern: "^\\w+-\\w+.*"
|
||||
replace: '[JIRA $0] '
|
||||
`,
|
||||
expectedDidChange: true,
|
||||
},
|
||||
{
|
||||
name: "Complicated CommitPrefixes Rename",
|
||||
|
|
@ -50,15 +52,14 @@ func TestCommitPrefixMigrations(t *testing.T) {
|
|||
- pattern: "^foo.bar*"
|
||||
replace: '[FUN $0] '
|
||||
`,
|
||||
expectedDidChange: true,
|
||||
},
|
||||
{
|
||||
name: "Incomplete Configuration",
|
||||
input: "git:",
|
||||
expected: "git:",
|
||||
name: "Incomplete Configuration",
|
||||
input: "git:",
|
||||
expectedDidChange: false,
|
||||
},
|
||||
{
|
||||
// This test intentionally uses non-standard indentation to test that the migration
|
||||
// does not change the input.
|
||||
name: "No changes made when already migrated",
|
||||
input: `
|
||||
git:
|
||||
|
|
@ -69,37 +70,33 @@ git:
|
|||
foo:
|
||||
- pattern: "^\\w+-\\w+.*"
|
||||
replace: '[JIRA $0] '`,
|
||||
expected: `
|
||||
git:
|
||||
commitPrefix:
|
||||
- pattern: "Hello World"
|
||||
replace: "Goodbye"
|
||||
commitPrefixes:
|
||||
foo:
|
||||
- pattern: "^\\w+-\\w+.*"
|
||||
replace: '[JIRA $0] '`,
|
||||
expectedDidChange: false,
|
||||
},
|
||||
}
|
||||
|
||||
for _, s := range scenarios {
|
||||
t.Run(s.name, func(t *testing.T) {
|
||||
actual, err := computeMigratedConfig("path doesn't matter", []byte(s.input))
|
||||
actual, didChange, err := computeMigratedConfig("path doesn't matter", []byte(s.input))
|
||||
assert.NoError(t, err)
|
||||
assert.Equal(t, s.expected, string(actual))
|
||||
assert.Equal(t, s.expectedDidChange, didChange)
|
||||
if didChange {
|
||||
assert.Equal(t, s.expected, string(actual))
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestCustomCommandsOutputMigration(t *testing.T) {
|
||||
scenarios := []struct {
|
||||
name string
|
||||
input string
|
||||
expected string
|
||||
name string
|
||||
input string
|
||||
expected string
|
||||
expectedDidChange bool
|
||||
}{
|
||||
{
|
||||
name: "Empty String",
|
||||
input: "",
|
||||
expected: "",
|
||||
name: "Empty String",
|
||||
input: "",
|
||||
expectedDidChange: false,
|
||||
},
|
||||
{
|
||||
name: "Convert subprocess to output=terminal",
|
||||
|
|
@ -111,6 +108,7 @@ func TestCustomCommandsOutputMigration(t *testing.T) {
|
|||
- command: echo 'hello'
|
||||
output: terminal
|
||||
`,
|
||||
expectedDidChange: true,
|
||||
},
|
||||
{
|
||||
name: "Convert stream to output=log",
|
||||
|
|
@ -122,6 +120,7 @@ func TestCustomCommandsOutputMigration(t *testing.T) {
|
|||
- command: echo 'hello'
|
||||
output: log
|
||||
`,
|
||||
expectedDidChange: true,
|
||||
},
|
||||
{
|
||||
name: "Convert showOutput to output=popup",
|
||||
|
|
@ -133,6 +132,7 @@ func TestCustomCommandsOutputMigration(t *testing.T) {
|
|||
- command: echo 'hello'
|
||||
output: popup
|
||||
`,
|
||||
expectedDidChange: true,
|
||||
},
|
||||
{
|
||||
name: "Subprocess wins over the other two",
|
||||
|
|
@ -146,6 +146,7 @@ func TestCustomCommandsOutputMigration(t *testing.T) {
|
|||
- command: echo 'hello'
|
||||
output: terminal
|
||||
`,
|
||||
expectedDidChange: true,
|
||||
},
|
||||
{
|
||||
name: "Stream wins over showOutput",
|
||||
|
|
@ -158,6 +159,7 @@ func TestCustomCommandsOutputMigration(t *testing.T) {
|
|||
- command: echo 'hello'
|
||||
output: log
|
||||
`,
|
||||
expectedDidChange: true,
|
||||
},
|
||||
{
|
||||
name: "Explicitly setting to false doesn't create an output=none key",
|
||||
|
|
@ -170,14 +172,18 @@ func TestCustomCommandsOutputMigration(t *testing.T) {
|
|||
expected: `customCommands:
|
||||
- command: echo 'hello'
|
||||
`,
|
||||
expectedDidChange: true,
|
||||
},
|
||||
}
|
||||
|
||||
for _, s := range scenarios {
|
||||
t.Run(s.name, func(t *testing.T) {
|
||||
actual, err := computeMigratedConfig("path doesn't matter", []byte(s.input))
|
||||
actual, didChange, err := computeMigratedConfig("path doesn't matter", []byte(s.input))
|
||||
assert.NoError(t, err)
|
||||
assert.Equal(t, s.expected, string(actual))
|
||||
assert.Equal(t, s.expectedDidChange, didChange)
|
||||
if didChange {
|
||||
assert.Equal(t, s.expected, string(actual))
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
@ -786,20 +792,21 @@ keybinding:
|
|||
|
||||
func BenchmarkMigrationOnLargeConfiguration(b *testing.B) {
|
||||
for b.Loop() {
|
||||
_, _ = computeMigratedConfig("path doesn't matter", largeConfiguration)
|
||||
_, _, _ = computeMigratedConfig("path doesn't matter", largeConfiguration)
|
||||
}
|
||||
}
|
||||
|
||||
func TestAllBranchesLogCmdMigrations(t *testing.T) {
|
||||
scenarios := []struct {
|
||||
name string
|
||||
input string
|
||||
expected string
|
||||
name string
|
||||
input string
|
||||
expected string
|
||||
expectedDidChange bool
|
||||
}{
|
||||
{
|
||||
name: "Incomplete Configuration Passes uneventfully",
|
||||
input: "git:",
|
||||
expected: "git:",
|
||||
name: "Incomplete Configuration Passes uneventfully",
|
||||
input: "git:",
|
||||
expectedDidChange: false,
|
||||
},
|
||||
{
|
||||
name: "Single Cmd with no Cmds",
|
||||
|
|
@ -810,6 +817,7 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) {
|
|||
allBranchesLogCmds:
|
||||
- git log --graph --oneline
|
||||
`,
|
||||
expectedDidChange: true,
|
||||
},
|
||||
{
|
||||
name: "Cmd with one existing Cmds",
|
||||
|
|
@ -823,6 +831,7 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) {
|
|||
- git log --graph --oneline
|
||||
- git log --graph --oneline --pretty
|
||||
`,
|
||||
expectedDidChange: true,
|
||||
},
|
||||
{
|
||||
name: "Only Cmds set have no changes",
|
||||
|
|
@ -830,10 +839,7 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) {
|
|||
allBranchesLogCmds:
|
||||
- git log
|
||||
`,
|
||||
expected: `git:
|
||||
allBranchesLogCmds:
|
||||
- git log
|
||||
`,
|
||||
expected: "",
|
||||
},
|
||||
{
|
||||
name: "Removes Empty Cmd When at end of yaml",
|
||||
|
|
@ -846,6 +852,7 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) {
|
|||
allBranchesLogCmds:
|
||||
- git log --graph --oneline
|
||||
`,
|
||||
expectedDidChange: true,
|
||||
},
|
||||
{
|
||||
name: "Migrates when sequence defined inline",
|
||||
|
|
@ -856,6 +863,7 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) {
|
|||
expected: `git:
|
||||
allBranchesLogCmds: [baz, foo, bar]
|
||||
`,
|
||||
expectedDidChange: true,
|
||||
},
|
||||
{
|
||||
name: "Removes Empty Cmd With Keys Afterwards",
|
||||
|
|
@ -870,14 +878,18 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) {
|
|||
- git log --graph --oneline
|
||||
foo: bar
|
||||
`,
|
||||
expectedDidChange: true,
|
||||
},
|
||||
}
|
||||
|
||||
for _, s := range scenarios {
|
||||
t.Run(s.name, func(t *testing.T) {
|
||||
actual, err := computeMigratedConfig("path doesn't matter", []byte(s.input))
|
||||
actual, didChange, err := computeMigratedConfig("path doesn't matter", []byte(s.input))
|
||||
assert.NoError(t, err)
|
||||
assert.Equal(t, s.expected, string(actual))
|
||||
assert.Equal(t, s.expectedDidChange, didChange)
|
||||
if didChange {
|
||||
assert.Equal(t, s.expected, string(actual))
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue