-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Allow users to add new Control classes without implementing a plugin #2122
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
Conversation
1762475
to
40167e6
Compare
Closes #1992 |
@ocefpaf can you have a look? |
control: str | ||
The javascript class name of the control to be rendered. | ||
position: str | ||
One of "bottomright", "bottomleft", "topright", "topleft" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One problem that we have in folium templates are the silent erros when passing keywords to it. For example, if someone types bottonright
instead of bottomright
, it will take user a while to figure out what is wrong.
Maybe we should check for this 4 valid kw? I know this doesn't solve it for all templates, but in this case it may be small enough that it is worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean as a static typing check or as an assert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid assert
use outside of tests.
I'm old school but here is what I would do:
- make position a kw argument, not an arg;
- Sanitize it with a
position.lower()
to validate names copied from leaflet JS docs; - Check if the sanitized positions is in a valid list with those 4 names and raise a
ValueError
if not.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we have been inconsistent in banning assert
:-)
40167e6
to
cf0e881
Compare
|
||
if position is not None: | ||
position = position.lower() # type: ignore | ||
if position not in (args := get_args(TypePosition)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I was a Fortran programmer in my early days, type checking is not my forte. (Heck, I moved to Python to get away from this ;-p)
My crude review tells me that this is equivalent to what I described in my review comment, right? If so, merge away!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote my first program for money in RPG :-) which is a fairly obscure language almost as old as Fortran. Whitespace is significant to a fault.
C* PARAMETER LIST
C *ENTRY PLIST
C PARM DATEIN 80
C* PROGRAM START
C DATEIN DIV 100 DATEIN
C MVR DAY 20
C DATEIN DIV 100 DATEIN
C MVR MONTH 20
C Z-ADDDATEIN YEAR 40
C* SHOW RESULTS
C YEAR DSPLY
C MONTH DSPLY
C DAY DSPLY
C SETON LR
C* EXECUTION EXAMPLE
C* CALL SEPDATE PARM(X'019960531F')
I added one more test.
@Conengmo
This is a new, smaller implementation of the
Control
class I implemented earlier. For this PR I focused only on the user's ability to load a new control in user space, without having to write a new plugin. It is not difficult to write a plugin, but it does require knowledge of Jinja. Also, there is quite some boilerplate code involved.With this
Control
clas I was able to implement theminimap
andruler
plugins, using just a few lines of code.