-
Notifications
You must be signed in to change notification settings - Fork 180
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
Add type checking functions. #43
Conversation
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.
Thanks, this looks very straightforward! Just a couple comments.
lib/types.bzl
Outdated
@@ -0,0 +1,57 @@ | |||
# Copyright 2017 The Bazel Authors. All rights reserved. |
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.
2018
lib/types.bzl
Outdated
return type(v) == type(True) | ||
|
||
|
||
types = struct( |
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.
For completeness, could you go ahead and implement a function for each of the data types listed in the Skylark spec? That would round this out very nicely. https://docs.bazel.build/versions/master/skylark/spec.html#data-types
awesome, thanks for such a supersonic review @allevato! I'll update the PR with copyright update and the rest of type checks! |
added all missing type checks and fixed copyright, please take a another look. Thanks! |
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 extremely nitpicky word choice comment, but otherwise this looks good to me. Thanks for your contribution!
@brandjon , do you have any comments you'd like to add about this?
lib/types.bzl
Outdated
|
||
|
||
def _is_list(v): | ||
"""Returns True if v is an instance of the list. |
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.
Super-nit: Here and in the Returns
clause, can you say "a list", etc. instead of "the list"? (Throughout the docs for each of these functions.)
Even though it's not great to use type checks, they are frequently useful for checking input types of macros. Because there is no standard way of checking types, at least 2 types of checks are used: - `type(foo) == type([])` - `type(foo) == "list"` The first option is not very readable and the second option seem to be relying on an Bazel implementation detail. Encapsulating type checks into this library enables consistent and easy to understand type checking without explicitly relying on implementation details.
thanks for the review @allevato! I was going back and forth between |
Thanks! I also wanted a library like this :) |
LGTM |
Thanks for your contribution, @ttsugriy! (Laurent beat me to merging it by 30 seconds... 🙂) |
thanks a lot for such a thorough and supersonic review! |
Even though it's not great to use type checks, they are frequently useful for
checking input types of macros.
Because there is no standard way of checking types, at least 2 types of checks
are used:
type(foo) == type([])
type(foo) == "list"
The first option is not very readable and the second option seem to be relying
on an Bazel implementation detail. Encapsulating type checks into this library
enables consistent and easy to understand type checking without explicitly
relying on implementation details.
There are still more checks to add, but this change creates a scaffold.