Output of breeze kvstore nodes is broken #14
Comments
|
Thanks @tamihiro for updating request, it is much clear now that what was broken and how it is fixed. I am initiating pull, once it passes all tests in our internal CI build, it will automatically get pushed to Github and will be closed. Cheers, |
|
Thanks @saifhhasan for the follow up! |
facebook-github-bot
pushed a commit
that referenced
this issue
May 5, 2021
Summary: Fix error in breeze decision: ``` # pyre-fixme[14]: `_run` overrides method defined in `OpenrCtrlCmd` inconsistently. ``` Reference: https://www.internalfb.com/intern/staticdocs/pyre/docs/errors/#1415-behavioral-subtyping NOTE: Parameter types can't be more restrictive and return types can't be more permissive in overridden methods. The overriding function need to accept all arguments that the overridden function can. --- The error happens because the superclass defines this _run() method like: ``` # The overriding function in superclass def _run(self, client: OpenrCtrl.Client, *args, **kwargs) -> None: """ To be implemented by sub-command """ raise NotImplementedError ``` `*args, **kwargs` allows you to write functions with arbitrary number of arguments. When the subclass overrides this, it cannot be more restrictive than the superclass which means the subclass needs to allow the same arbitrary number of arguments. --- So we have two ways to fix this: 1: As this diff, add the `*args,**kwargs,` as the parameters for overridden methods in the subclass. 2: Add all potential parameters and delete the `*args,**kwargs,` in the superclass. For the current codebase of this case, we need to add the parameters including but not limited to: ``` # The overriding function in superclass def _run(self, client: OpenrCtrl.Client, nodes: set, # for DecisionPrefixesCmd, DecisionRoutesComputedCmd, DecisionAdjCmd json: bool, # for DecisionPrefixesCmd, DecisionRoutesComputedCmd, DecisionAdjCmd, KvStoreCmdBase (defined as `in_json`)... prefix: str, # for DecisionPrefixesCmd, DecisionRoutesComputedCmd (defined `Any` type) client_type: str, # for DecisionPrefixesCmd labels: Any, # for DecisionRoutesComputedCmd areas: set, # for DecisionAdjCmd, PathCmd, DecisionValidateCmd... bidir: bool, # for DecisionAdjCmd, AdjCmd src: str, # for PathCmd dst: str, # for PathCmd max_hop: int, # for PathCmd file: str, # for ConfigDryRunCmd key: str, # for ConfigEraseCmd, ConfigStoreCmd keys: List[str], # for KvKeyValsCmd value: str, # for ConfigStoreCmd detailed: bool, # for ReceivedRoutesCmd prefix_or_ip: List[str] # for FibUnicastRoutesCmd duration: int, # for FibSnoopCmd initial_dump: bool, # for FibSnoopCmd prefixes: List[str], # for FibSnoopCmd originator: Any = None, # for KvKeysCmd ttl: bool = False, # for KvKeysCmd interface: Any, # for KvShowAdjNodeCmd ...... # more to come~~ ) -> None: """ To be implemented by sub-command """ raise NotImplementedError ``` but everytime the overridden function has a new parameter, we need to modify the overriding function. {emoji:1f61b} If #1 (this diff) is good, I will make the similar changes in other modules. Reviewed By: xiangxu1121 Differential Revision: D28204319 fbshipit-source-id: 4c07be177040fb31ae3d449ce29961b0e933696c
facebook-github-bot
pushed a commit
that referenced
this issue
May 7, 2021
Summary: Fix all overriding signature errors in breeze. Check D28204319 (b00fc6e) for more details. Reviewed By: saumitrp, xiangxu1121 Differential Revision: D28239072 fbshipit-source-id: 1ba9b6b9a0e631823196c71638d93118b9717f16
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Issue Description
BreezeCLI returns nodes information in KvStore, but the output is pretty much broken.Environment
Minimal test code / Steps to reproduce the issue
sudo run_openr.shbreeze kvstore nodesWhat's the actual result?
What's the expected result?
Solution suggested
Pull request #12
The text was updated successfully, but these errors were encountered: