Move constants to separate file

[cdce8p]

Storing constants in one or multiple classes is inefficient in python. A better way is to create a separate file and import the necessary constants.
Furthermore I added additional constants for reoccurring variables, mainly for the HAP representation and properties.

I've left out the hap_server module, since it quite possibly will change with the conversion to async, so constants for it can be added later.

As a side notes:

• I've changed the file permissions and removed execute as program
• update_advertisment is now update_advertisement

I will rebase this PR as soon as #85 is merged. Otherwise there might be merge conflicts.

[ikalchev]

The reason I split the constants across modules is because most of them are used only in the modules they are contained. For example the constants in the char module which relate to the format, etc. are used exclusively there. What is the reason to move such constants to a common file?

[cdce8p]

A few reasons:

• I've mentioned earlier: Classes are a quite inefficient way of storing just some constants
• It add some future value, since you don't need to remember where you have defined a constant if you need it later. E.g. the STANDALONE_AID was defined in accessory but also used in accessory_driver.
• It makes the existing module easier to read. Fewer lines.
• Prevention of spelling mistakes for newly added constants
• Easier to import them for other implementations if it's clear they are all in const.py

IMO especially the second to last argument is important. Although additional constants could have been added in the individual modules as well, they are more clearly structured and readable in a separate one.

[ikalchev]

I agree with the classes point, they are more readable but terrible otherwise. But this aside - why pollute one namespace with stuff that is used only in another (like the char format constants). Furthermore, about point 2 - if I need to use the char format constants and I don’t know where they are, my first thought would be to look in the chars module for them. IMO, I would expect them to be there because it’s their logical place.

Point 3 - fewer lines is not always easier. For example I need to open another file to see the definition of a constant. And that other file is longer and contains a lot of different sets of constants, which makes it hard to follow.

Point 4 - spelling mistakes are equally likely in both cases I think. But if the constants are in the same module in which they are used the most, they will be more convenient for reading (again IMO)

Lastly, point 5 - other implementations often need a specific subset of functionalities at any given moment. If a third party module wants to do server stuff it needs to understand only the server module. Why then import from a file containing all constants which it does not care about? Also, whether you do from pyhap.constants import foo or from pyhap.characteristics import foo is equally easy. But in the later case, the reader knows that foo is something char-related, it gives context.

[cdce8p]

I can probably counter those arguments (and will later on), but what it comes done to is a philosophical question on wheather or not a seperate constants file is needed for a project this size. As you know, I come from Home Assistant and that's the way we do things there and I personally see certain benefits in it which is why I suggested the changes in the first place.

Regarding the namespace: With using from .const import (...) only those constants specified really get imported, so you don't have any issues there.

Point 2: That might be true if constants are used in one module only. However this is not always the case although the grade varies.

Point 3: The idea behind constants is that the name should tell you everything you need to know about them. E.g. you shouldn't care if it's minValue, MinValue or min_value. Furthermore a separate module can easily opend in split screen mode to view them beside the original module.

Point 4: Spelling in this case is meant for writing 'characteristics' multiple times for the HAP representation. If you make a mistake yu would only notice it during runtime. Sometimes faster sometimes slower. With constants a speeling mistake would lead to a ValueError or ImportError during complation. So it's a bit more likely to be found fast.

Point 5: Your're right here. However to some degree this is managed be giving fitting names like HAP_FORMAT_****, etc.

[ikalchev]

Point 2: this is exactly the case with the server and char format constants. In fact, the only constants used outside the module they are defined in are the AID and Category (I am not considering the newly added constants for REPR, these clearly need to go in a consts file).

Point 3: I don’t see how this is a plus for either side of the argument, in both cases the constant is just a name you use, regardless of where it is defined.

Point 4: Looking at this and point 3, I get the impression that you think I am against constants and I am not. My only concern is in combining all constants in a single file, although the majority of them are not used outside a specific module. Obviously there are constants that are used across modules, I don’t oppose placing such in a single file.

Let me also add that different projects have different style guides. The same way that you see benefits in the single module approach, I see benefits in keeping constants close to what they refer to. Vice versa, and I think you can agree, both have drawbacks.

[cdce8p]

To find some common ground here, what do you think of the following:

• I move all constants related to a single module (mainly HAP_FORMAT_***, HAP_PERMISSION_***, HAP_UNIT_*** and PROP_***) to their files, but leave out the separate classes to improve performance.
• The HAP_REPR_*** constants stay in const.py

What's your opinion on the CATEGORY constants?

---

Update:
HAP_PERMISSION_** are also used by the accessory_driver, so I would prefer to leave them in const.py as well.

[ikalchev]

I am fine with this. As for the CATEGORY, I would prefer it stays in Accessory but I don't mind moving it to consts

[cdce8p]

I moved CATEGORY back to the accessory file.

[ikalchev]

I thought you wanted to change this to dict?

[cdce8p]

I would have loved to. However I don't think that it quite works here. At the moment this version seems to be the best.

For the future we could think about what is stored in the accessory. As far as I can tell, it's the category number at the moment. With a change to a dictionary it might make sense to store the constant name and assign the number later when sending the acc infos to HomeKit.
I didn't wanted to have this discussion here which is the reason I changed it back, so this could get merged.

[ikalchev]

What is the purpose of having it like e.g. string in the Acc? Readability?

[cdce8p]

Using a dictionary might pose problems when importing a category.
...

While I was thinking about it, are their problems with just doing the following?

CAT_OTHER = 1
CAT_BRIDGE = 2
# ...

That would solve the import problem and doesn't need a dict.

---

For comparison the dict variant I thought of was:

CAT_OTHER = 'other'
CAT_BRIDGE = 'bridge'

CATEGORIES = {
    CAT_OTHER: 1,
    CAT_BRIDGE: 2,
}

[ikalchev]

No problems, I would just name it CATEGORY_OTHER, etc.

[cdce8p]

Would you mind if I move them to the const file?

[ikalchev]

Go ahead

[codecov-io]

Codecov Report

Merging #86 into dev will decrease coverage by 6.85%.
The diff coverage is 61.44%.

@@            Coverage Diff             @@
##              dev      #86      +/-   ##
==========================================
- Coverage   36.78%   29.93%   -6.86%     
==========================================
  Files          12       13       +1     
  Lines        1283     1306      +23     
  Branches      130      128       -2     
==========================================
- Hits          472      391      -81     
- Misses        807      915     +108     
+ Partials        4        0       -4

Impacted Files	Coverage Δ
pyhap/service.py	30% <100%> (-21.36%)	⬇️
pyhap/const.py	100% <100%> (ø)
pyhap/accessory.py	34.04% <100%> (-13.2%)	⬇️
pyhap/accessory_driver.py	21.19% <15.78%> (+0.36%)	⬆️
pyhap/characteristic.py	43% <64.44%> (-54.06%)	⬇️
pyhap/loader.py	33.33% <0%> (-14.17%)	⬇️
pyhap/util.py	40.9% <0%> (-2.28%)	⬇️
... and 2 more