Fix support for display hints being applied #7
Conversation
While recent commit git-84be3ff resolved an issue with stacked textual conventions, it broke support for display hints, by changing the method resolution order (MRO) of types based on textual conventions. The orginal MRO was necessary for pysnmp to apply display hints. This commit replaces the previous solution with a new one. The new fix preserves the original MRO and also avoids subclassing the TextualConvention class multiple times for any type, thereby resolving both the original issue and the one newly introduced by git-84be3ff. This new fix does (necessarily) add to "recompilation creep": in order to know whether or not to subclass TextualConvention for a new type, information needs to be carried over between MIBs about whether imported types are simple types or textual conventions. That means that if one MIB is changed (e.g. by changing a type from simple to textual convention or vice versa), the MIBs importing that MIB may have to be recompiled as well. pysmi is not yet smart enough to know about that, and may therefore skip recompiling the importing MIBs, leading to a possible desynchronization. That same issue was already present for handling of DEFVALs as well. New tests are added to ensure that display hint information is indeed not only available, but also applied. Please note that this commit extends the pysmi JSON output format.
|
But now no test cases cover Unsigned32 related display hints, right? @dcvmoole Do you plan to work on that in another pull request? |
|
Not for pysmi. The reason I changed Unsigned32 to INTEGER in the existing test here, is that in order to process a DEFVAL value for an Unsigned32 syntax, pysmi requires actual symbol information from SNMPv2-SMI (i.e., it needs to do a symbol table pass on that entire MIB first), and SNMPv2-SMI is not available to the test set. However, Unsigned32 was an arbitrary choice for that test in the first place: for pysmi, handling of DISPLAY-HINT clauses is fully independent of the specific syntax, so for the purpose of this test, any base type will do. It would of course be possible to make a copy of the previous test and change/add the INTEGER/DEFVAL parts in the copy only, but that would just be redundant and add no value for pysmi. On the other hand, it would definitely be useful to have a full sweep of display hint processing against each base type, especially in the light of the issues that came up in lextudio/pysnmp#139. However, that is conceptually outside of the scope of pysmi and its test set, and would practically run into the above symbol table issue anyway. I think it would make much more sense to add such a test to pysnmp, as it involves mostly functionality on the pysnmp side. Given that I currently do not foresee having any use for the pysnmp display hint functionality myself, I have no plans for working on that at this time. |
|
Thanks. Shipped in release 1.5.6. |
This change restores support in pysmi for applying display hints, as was broken by 84be3ff. This is the pysmi issue I mentioned in lextudio/pysnmp#139. Of course, the display hint related issues in pysnmp itself still remain.
As before, this change has been tested with the script from PR #3, now with the updated mibs.pysnmp.com collection. The results are exactly the same before and after this change. From pysmi's perspective, the main observable difference of this change is that the newly added "PrettyValue" test cases failed before, and pass after this change.
Please note that when using the latest pysnmp HEAD, the pysmi test set already started to fail for other reasons (namely, the rename of namedValues). I don't know whether that will be an issue for automatic verification here.
Also, a heads-up FWIW: I intend to submit a few more pysmi changes later this week, with as ultimate goal the ability to regenerate the code of the base MIBs in pysnmp.