search

apache / incubator-ponymail

Apache Pony Mail (Incubating) - Email for Ponies & People
http://ponymail.incubator.apache.org/
Other
80 stars 30 forks source link

setup: move mappings to a separate file #467

Closed dimidd closed 6 years ago

dimidd commented 6 years ago

Signed-off-by: Dimid Duchovny dimidd@gmail.com

sebbASF commented 6 years ago

Please don't mix changes in a single PR; this makes it harder to review.

Also, I'm not convinced that there is a need for the feature.

And unfortunately JSON does not support comments, unlike Python. If there is support for the customisation, I would prefer to see it implemented differently, eg by allowing the embedded settings to be overridden by an option that reads a separate file.

dimidd commented 6 years ago

OK, I'll make a separate PR for whitespace fixes. Regarding comments, how about using YAML which does support one-line comments? I agree that the best option would be to add an optional flag --mappings_file <my_mappings.yml>, to override the default one. Some projects even force the user to customize, by providing only an example file, but I didn't wan't to break existing scripts and docs. In addition, setup.py is way trop long, and I believe that outsourcing the mappings would also improve readability.

sebbASF commented 6 years ago

I'm still not convinced that there is a general use case for a separate mappings file. What are the changes you want to make? Can you not adjust the mappings after running setup?

As to simplifying setup, another approach might be to put the database setup in a separate Python module. That would allow comments, and allow conditional mapping depending on the DB version.

But at present, setup.py is working, and I don't see any pressing need to fix what ain't broken.

dimidd commented 6 years ago

OK, if it's not relevant for other Pony users, I guess it's not worth the trouble.