Options cleanup. #61
Options cleanup. #61
Conversation
- intoduce a type alias, making code lines significantly shorter.
- this also serves as a default constructor, since {Any, Any} are the
default parameters
- move all options-related code to a single file (no substantial
changes otherwise)
- add a few docstrings, especially @pgf
Codecov Report
@@ Coverage Diff @@
## master #61 +/- ##
=======================================
Coverage 77.75% 77.75%
=======================================
Files 8 9 +1
Lines 472 472
=======================================
Hits 367 367
Misses 105 105
Continue to review full report at Codecov.
|
src/options.jl
Outdated
| Contents emitted in `key = value` form, or `key` when `value ≡ nothing`. Also | ||
| see the [`@pgf`](@ref) convenience macro. | ||
| """ | ||
| const Options = OrderedDict |
There was a problem hiding this comment.
Shouldn't this be OrderedDict{Any, Any} for it to be concrete?
There was a problem hiding this comment.
I don't understand what the advantage of it being concrete would be, can you please clarify?
This way I can use it for both the constructor (empty Options()) and dispatch. Perhaps I should use an UnionAll type explicitly?
There was a problem hiding this comment.
It probably doesn't matter. Would save dynamic dispatch when accessing the options field but it shouldn't really have any relevance.
There was a problem hiding this comment.
But I don't understand why you couldn't use it for a constructor and dispatch even if it was concrete?
There was a problem hiding this comment.
Since in the original code some methods were dispatched on ::OrderedDict, I thought that the implicit expectation was that we would take any <: OrderedDict, hence the UnionAll type.
If that's not required, I can make it concrete, no problem.
There was a problem hiding this comment.
I think they will always be {Any, Any}.
There was a problem hiding this comment.
At least if we use the Option type everywhere, they will be by construction.
src/options.jl
Outdated
| `key = value`, `key : value`, or `key => value` pairs enclosed in `{ ... }`, | ||
| anywhere in the expression. | ||
|
|
||
| Multi-word keys need to be quoted. |
There was a problem hiding this comment.
Actually, you can use e.g. only_marks for this.
|
I thought about it and think that the cleanest solution would be that instead of an alias, to have a thin wrapper struct Options
options::OrderedDict{Any, Any}
endwith neat |
|
Could do that. Not sure how much it matters, the |
|
OK, made it concrete, will shelve the options wrapper for another day, so I am ready to merge; should I wait for #62? They look very orthogonal and I think it will be clean in any order. |
|
Rebased with online editor... let's see if I made it. |
|
Nope, failed. Should work now. |
test/test_elements.jl
Outdated
| @test pgf.Table(x, y, z) ≅ pgf.Table(pgf.Options(), | ||
| hcat(pgf.matrix_xyz(x, y, z)...), | ||
| @test Table(x, y, z) ≅ Table(PGFPlotsX.Options(), | ||
| hcat(PGFPlotsX..matrix_xyz(x, y, z)...), |
There was a problem hiding this comment.
looks like an extra . here
There was a problem hiding this comment.
Aaargh, it is impossible to use the online editor. Never again.
hopefully this runs now
intoduce a type alias, making code lines significantly shorter.
this also serves as a default constructor, since
{Any, Any}are the default parametersmove all options-related code to a single file (no substantial changes otherwise)
add a few docstrings, especially
@pgf