Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter Members Exposed via Control Interface #213

Merged
merged 7 commits into from
Mar 16, 2017

Conversation

MikeHart85
Copy link
Contributor

Fixes #194.

Filters members exposed by default (not overridden via the members param) via ExposedObject.

Members filtered, in addition to the previous "starting with _" condition, are anything defined or inherited in Device or StateMachineDevice and specifically "log".

log had to be done explicitly since it is added dynamically by a decorator.

Also fixed an error in control.py... not sure how that slipped past both of us! 馃槅

Copy link
Contributor

@MichaelWedel MichaelWedel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested a bit and found that now all the values are gone from lewis-control (see the comment above).

Also, the unit tests for ExposedObject need to be updated.

@@ -48,7 +48,7 @@ def show_api(remote, object_name):
raw_value = str(getattr(obj, prop))
value_lines = raw_value.split('\n')

current_value = value_lines[0][40:] + (
current_value = value_lines[0][:40] + (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, really weird that we did not see it earlier. But it's not enough to just change this, now all the values are gone if the string is below 40 characters. It took me a while to see that and I still don't quite understand what's going on,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you describe is the behaviour I was getting before making this change... it's exactly what it's supposed to fix (:40 means "from beginning to index 40", 40: means "from index 40 to end").

Are you sure you're using the correct version of lewis-control?

If you have lewis installed, you have to be careful to use $ ./lewis-control.py, and not simply $ lewis-control... otherwise you're using the installed version without this change. I think this may also be how this slipped passed us in the first place?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might be right, I just tried again on a different machine being careful to use the right script. So forget everything I wrote except for "good catch" and add another "good catch".

prop.startswith('_'),
prop == 'log',
prop in dir(Device),
prop in dir(StateMachineDevice)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think getting the device bases classes and 'log' in here is necessary, it just makes the control_server depend on this one very specific thing. I played around a bit and I think it can be made entirely general, what do you think about this:

    def __init__(self, obj, members=None, exclude=None, expose_bases=False):
        super(ExposedObject, self).__init__()

        self._object = obj
        self._function_map = {}

        self._add_function(':api', self.get_api)

        exposed_members = members if members else self._public_members()
        exclude_final = list(exclude or []) + dir(inspect.getmro(type(obj))[1]) if not expose_bases else []

        for method in exposed_members:
            if not exclude_final or method not in exclude_final:
                self._add_member_wrappers(method)

This automatically catches 'log' for all subclasses of DeviceBase because it's in the dict of that type. For simulation and interfaces it's necessary to list 'log' in Simulation explicitly where they are exposed, but in my opinion that's the "lesser evil", because it keeps ExposedObject general.

@MikeHart85
Copy link
Contributor Author

Great review suggestion!

Made updates accordingly, with some tweaks as discussed on slack.

@MichaelWedel MichaelWedel merged commit 2146296 into master Mar 16, 2017
MikeHart85 added a commit that referenced this pull request Mar 16, 2017
MichaelWedel added a commit that referenced this pull request Mar 17, 2017
Due to #212 and #213 in short sequence, this new member was not in the exclude list yet.
@MichaelWedel MichaelWedel deleted the 194_clean_control_server_interface branch March 20, 2017 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More selective remote exposure of attributes/methods
2 participants