Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid config file gets replaced by default #36

Closed
jsubhi opened this issue Mar 11, 2019 · 6 comments
Closed

Invalid config file gets replaced by default #36

jsubhi opened this issue Mar 11, 2019 · 6 comments
Labels
bug Something isn't working enhancement New feature or request fixed Issue is fixed

Comments

@jsubhi
Copy link
Collaborator

jsubhi commented Mar 11, 2019

level: high
If the config.json file is invalid (due to an editing error) it gets replaced by the default config without any warning. This should not happen! Instead the Program should quit with an appropriate error message (preferably giving a hint about what is wrong with the file).

@jsubhi jsubhi added the bug Something isn't working label Mar 11, 2019
@EliDeh
Copy link
Collaborator

EliDeh commented Mar 12, 2019

the config.json file only get replaced if the key 'defaultDownloadPath' can not be found.

@EliDeh EliDeh added enhancement New feature or request and removed bug Something isn't working labels Mar 12, 2019
@jsubhi
Copy link
Collaborator Author

jsubhi commented Mar 22, 2019

The file is always replaced if the json is invalid.
e.g. removing the comma after
"sortByPPN": false,
will trigger replacement of the file upon next start of the app.
Orr adding an extra backslash to the regex:
This is OK: "regex": "^([A-Z]) (.):(.)$",
This will trigger replacement: "regex": "^([A-Z]
) (.):(.)$",

Not a big issue but irritating if I don't have a backup of the original config since any small typo will erase my data.

@jsubhi jsubhi added the bug Something isn't working label May 16, 2019
@jsubhi
Copy link
Collaborator Author

jsubhi commented May 16, 2019

Ich habe dies jetzt mal as bug hochgestuft. Ich habe gerade über eine halbe Stunde am Telefon verbracht, weil ich einer Kollegin helfen wollte, die Probleme mit der Konfiguration hatte. Irgendwo in der config.json war ein Fehler - wir wussten aber nicht wo. Nach jedem erfolglosen Korrekturversuch wurde unsere config.json durch die Defaultversion ersetzt sodass wir wieder unsere Version ins Verzeichnis kopieren mussten. Zusätzlich tauchen dann immer auch die default Formate auf, und das Programm beschwert sich wegen fehlender Drucker...
Fatal ist auch, dass man gar nicht mitbekommt, wenn die Datei ersetzt wird - man denkt, man hat eine Änderung gemacht, z.B. den Default Mode, die hat aber keinen Effekt, weil sie gleich überschrieben wurde.

Ich möchte sehr darum bitten, dass die config.json nur dann überschrieben wird, wenn die Datei gar nicht existiert. Andernfalls sollte das Programm einfach mit einer möglichst hilfreichen Fehlermeldung abbrechen.
(Sorry für den Rant, aber ich hatte durch dieses Verhalten des Programms einen sehr irritierenden Vormittag...)

EliDeh added a commit that referenced this issue Jul 24, 2019
@EliDeh EliDeh added the fixed on dev Issue is fixed on the dev branch label Jul 24, 2019
@jsubhi
Copy link
Collaborator Author

jsubhi commented Aug 8, 2019

Much better now! Personally I would prefer to just have the error message and maybe an option in that dialogue to create a clean config file - otherwise I have to rename the file in addition to fixing my type. (e.g. "Error in config.json. Do you want to create a clean sample config file? The current file will be saved as config_invalid.json" YES / NO). But that's a minor thing. Issue can be closed.

@EliDeh
Copy link
Collaborator

EliDeh commented Aug 26, 2019

Thank you for your feedback.
The clean config is needed to start the program.
Your proposed dialogue would either keep the config as it is and close the program, because it can not start without a valid config. Or rename the current config and create a new default config and proceed.
That dialogue feels like an unnecessary step to find the problem.
I think its better to compare both configs and in doubt edit the new config step by step (if the problem isn't obvious).

EliDeh added a commit that referenced this issue Aug 26, 2019
@jsubhi
Copy link
Collaborator Author

jsubhi commented Sep 3, 2019

The dialogue is good now. I wanted to keep the original config.json because in the case of an error I have to rename the file back to config.json in addition to fixing my error. But that would be a rare event in normal life since one would not continually mess around with config.json as I was doing during testing. So I will close this issue as it is really not that important and I can live fine with the current solution. Thanks!

@jsubhi jsubhi closed this as completed Sep 3, 2019
@EliDeh EliDeh added fixed Issue is fixed and removed fixed on dev Issue is fixed on the dev branch labels Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request fixed Issue is fixed
Projects
None yet
Development

No branches or pull requests

2 participants