-
-
Notifications
You must be signed in to change notification settings - Fork 432
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
Create static constants for the default semantic tags #4622
base: main
Are you sure you want to change the base?
Conversation
@andrewfg you might be able to use this |
9cd9a5f
to
5b02a6a
Compare
Signed-off-by: Jimmy Tanagra <[email protected]>
5b02a6a
to
d008c2f
Compare
If we can be sure that the "simple" tag names must be unique, we can simplify the constants to just the last part of it, i.e.
|
Signed-off-by: Jimmy Tanagra <[email protected]>
Umm. Actually I strongly prefer the constants WITH prefixes. When developing a binding, if you want to apply a tag on a thing then the tag must be an EQUIPMENT_xx tag. Or if you want to apply tags to a channel they must be POINT_xx and/or PROPERTY_xx tags. |
@jimtng here is an example piece of code from the Hue thing handler whereby the thing's semantic equipment tag gets set dynamically. Ideally I would pull in your EQUIPMENT_xx tags from this PR rather than using hand coded strings.
|
I believe that I understand your point. My thinking is that when you're setting a tag here, you are selecting the "end point" of the tag without dictating the exact hierarchy of the tag. Yes, it must be a certain type (Equipment vs Point / Property) but in here you are not dictating that Equipment Z must be a subcategory of Y which is the subcategory of X. So instead of stating here This frees up the ability later on to restructure Z within core to be either promoted or demoted e.g. to become EQUIPMENT_Z, EQUIPMENT_X_Z, or even EQUIPMENT_A_B_Z without having to make changes in all the consumer of the constants. It is still The advantage is simpler looking code. You know you want a It has a 1 to 1 equivalent of using a plain string vs constant. A possible compromise could be to create the constants as TYPE_ENDNAME without having the hierarchy. So we'll have
However this has the potential to be confusing. |
Signed-off-by: Jimmy Tanagra <[email protected]>
I've changed it to this scheme now. I recognise that this makes it very clear the type of the tag without getting into the hierarchy details. |
defaultTags.add(DefaultSemanticTags.LOCATION_GARAGE); | ||
defaultTags.add(DefaultSemanticTags.LOCATION_HOUSE); | ||
defaultTags.add(DefaultSemanticTags.LOCATION_SHED); | ||
defaultTags.add(DefaultSemanticTags.LOCATION_SUMMERHOUSE); |
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.
Maybe we could use the casing ("SummerHouse") to also separate words with underscore, so this would become LOCATION_SUMMER_HOUSE
, WDYT?
Exactly. That is all I am asking for. The idea of tag nesting is too complex to automate anyway. Another (much better) idea would be to put the different classes of tag constants into actually different java classes. So you would have four classes Equipment, Point, Property, Location that contain each their own tag constants. So instead of using EQUIPMENT_XXX one would use Equipment.XXX |
Signed-off-by: Jimmy Tanagra <[email protected]>
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.
@@ -60,6 +64,62 @@ def appendLabelsFile(FileWriter file, def line, String tagSet) { | |||
file.write("\n") | |||
} | |||
|
|||
def camelToUpperCasedSnake(def tag) { |
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.
Brilliant! One of my wishes (when I grow up) is to be able to do regexes like that..
* @author Generated from generateTagClasses.groovy - Initial contribution | ||
*/ | ||
@NonNullByDefault | ||
public class DefaultSemanticTags { |
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.
Yes, I like this structure. Thankyou.
Signed-off-by: Jimmy Tanagra <[email protected]>
This enables the default semantic tags to be referred to as constants by bindings to enable compile time check.