Closed defkev closed 7 months ago
Thanks a lot for the initial setup @defkev! As you can see I added a bunch of changes to the PR. My idea is that if we make it an array of objects it can expand more in the future, plus it's more in line with the expected format for the actual code that depends on the columns.
On top of that I also updated the docs to only explain the new way, for now the code still supports both, but in the long run I would like to remove this to keep things simple. So the code basically does a migration from an array of strings to an array of objects to only then handle the code.
This PR also made me realize how there is duplicate default config objects, so I managed to use rollup-json (and a file rename) so that we can import from the default public defaults instead. This way the default is managed in only one place.
I hope these changes make sense. Feel free to test them out and let me know if you experience any issues!
Why would you add an obsolete property (enabled) for something you want to configure? If you don't want to enable the row (enabled: false) just don't add it to the configuration!
In the config file enabled
is an optional property. If the user does not provide it the code will assume that the column should be enabled. Effectively I have given the user the option to quickly toggle a column on or off without removing any code. To make this clearer I can update the README to explain this.
you are making something very simple very complicated for absolutely no reason.
I think this code is actually easier to reason about than your initial suggestion. I see this to be easier in two ways:
config.json
logic. In your suggestion there is no easy way to add another value to the column configuration.Also config.json.defaults is called config.json.defaults because its not supposed to be a json BUT defaults unless you copy/move it to config.json. Its supposed to be a TEMPLATE not a CONFIG. You are just confusing the user, again for no reason.
I think i understand what you are trying to accomplish by moving the DEFAULTS out of the CODE into a FILE which is a BAD IDEA because people tend to REMOVE/RENAME files which will cause your CODE to stop working!
I can't account for people removing files from the public folder. Whether that's a JSON file or a Javascript file doesn't matter the user should always assume all files are needed. I made an attempt at keeping the json.defaults
extension but couldn't get it to work with rollup-json
so because of that I renamed it. From a developer perspective it's a pretty good gain to not have to worry about copying default values multiple times. Given these factors I decided to rename the file.
To further remedy this worry I decided to introduce rollup-plugin-copy
and move the defaults file to the src
folder. This way the code will import that default config from the src
folder, but the config.json.defaults
file will still be there for the user to review and copy.
In the end all of this is opinionated, as code often is. I value your contribution, I really do, but in the end I'm the maintainer of this project and if we can't have a constructive conversation about the code I will still make the final decision.
I hope to see your contributions in the future!
There is no point to quickly enable/disable a row in the config, thats what the checkbox in the settings dialog does already.
The config file is meant to give the user a way to change the hardcoded defaults without changing the source code, that is literally why i added it, not to replace the settings dialog.
You dont need any rollup or 3rd party libraries or whatever for this.
Less = More
I am not going to read the remaining wall of text because i frankly don't care.
Its your code, you can of course do what you want with it but i'd rather maintain my fork than approve wtf the fuck this garbage is.
Cheers
Closes #563 (see 2nd comment for example/documentation)