add default theme to the Theme companion object #74
Conversation
|
While this does fix issues where compilation breaks when refactoring or having to include the theme explicitly, the rendering could in fact change. So you could unexpectedly "break" the rendering of your plot, and the compiler will not give you any feedback. Im specifically thinking about the case were a custom theme has been defined, the code is then refactored and the custom theme is now accidentally out of scope. In that case there is no feedback from the compiler because its now finding the implicit in the companion object. Maybe there is some other solution. Creating a trait with an implicit default theme that can be overridden maybe ? Or getting rid of the implicit theme entirely at the top level. |
|
@chrisshafer @jacoblurye et al. thanks. Note this one of the first passes getting into the evilplot codebase. I am trying to critically think through @chrisshafer 's comment for the problem "A custom theme accidentally going out of scope". I find validity to it, but it is seems primarily useful from a library designer viewpoint rather than an end user. So it feels awkward to thrust that on the the end user. However, it is an interesting use case I didn't think about. And, for sure it is helpful to have the compiler check when incorrect code refactoring may happen. From this perspective, I understand why a Future does not include a default ExecutionContext. This is to make sure to remind the user that they should be thinking about the execution context and not just let the Future end up on some conflicting thread pool. However, I don't think this should be a requirement for an end user facing api for a simple Plotting utility. Here it feels like a default theme is more useful to be out of mind unless it is desirable to override. Hopefully, commit c0e3cdb will provide the best of both worlds. Default implicit them for users and if they want them, explicit compiler checks for library author. I have instead added an ExplicitImplicits marker like Trait on the development side in order to provide compiler errors when the implicit themes have not been explicitly passed through the code. By including this I was actually able to catch a bunch of EvilPlot cases that refactoring may have actually caused the incorrect implicit from being passed through. I also added a Spec so we add a simple test to evil plot that both demonstrates the ability of the compiler to catch this accidental out of scope "feature", so code changes like this PR in the future do not slip through. |
True, I guess it depends on what "typical" consumption looks like. How often will a library user want to specify their own theme ? Honestly as a user of it I have customized default themes on almost every plot, but maybe I'm a "power user" 😉 On one hand it simplifies consumption because the user no longer needs to worry about importing the default theme. On the other you loose compile time checks for custom implicits. In my opinion its fairly common for Scala libraries to require the import of one or more implicits. Its generally frowned upon to "default" implicit arguments, because it prevents the compiler from checking that your custom implicit is in scope. That being said if the general usage doesn't include customizing the theme, we should lean towards "defaulting" it. |
Implicits have a “very” complicated priority lookup scheme. For the most part I try to use them at either a VeryLow priority (within it’s own object) or at a VeryHigh precedence (local scope definition).
Including the default implicit in the companion object is an idiomatic and ideal use case of a default configuration type like this. If a user wants a specific theme they can add it; otherwise they don't even have to know about themes. Furthermore, the tutorials and getting up to speed examples can be much more approachable by removing the friction from these unrelated details, but still make them configurable if someone desires this tangential feature.