Skip to content

Conversation

arnecls
Copy link
Contributor

@arnecls arnecls commented Aug 15, 2017

The purpose of this pull request

We saw a problem with the conversion from mixed case keys to lower case keys when parsing the configs. In a nutshell we cannot really distinguish between keys that can be case sensitive and those that are not (nested modulator arrays for example).
As of that and because of the fact that unknown config keys will stop gollum from starting we decided to make the config case sensitive again. In addition to that we will now display a suggestion for unknown keys, so that broken configs can be fixed faster.

Checklist

  • make test executed successfully
  • docs updated

- this removes the need to convert all keys to lowercase
- if there is an unknwon config key, a suggestion will be given
@arnecls arnecls added the bug label Aug 15, 2017
@arnecls arnecls added this to the v0.5.0 milestone Aug 15, 2017
@arnecls arnecls self-assigned this Aug 15, 2017
@arnecls arnecls requested a review from msiebeneicher August 15, 2017 08:33
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 70.856% when pulling ef5b478 on CaseSensitiveConfig into 706e082 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 70.856% when pulling ef5b478 on CaseSensitiveConfig into 706e082 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 70.937% when pulling 102b536 on CaseSensitiveConfig into 706e082 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 70.856% when pulling 7942a68 on CaseSensitiveConfig into 706e082 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 70.856% when pulling 7942a68 on CaseSensitiveConfig into 706e082 on master.

Copy link
Contributor

@msiebeneicher msiebeneicher left a comment

Choose a reason for hiding this comment

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

Only one small change request - rest LGTM


func (conf PluginConfig) suggestKey(searchKey string) string {
closestDist := len(searchKey)
bestMatch := "something else"
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe better to use "no suggestion" or an empty string as default?

@arnecls arnecls merged commit f1386f8 into master Aug 15, 2017
@arnecls arnecls deleted the CaseSensitiveConfig branch August 15, 2017 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants