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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for two issues in View.include #42

Merged
merged 5 commits into from
Jul 11, 2017
Merged

Conversation

izapolsk
Copy link
Contributor

@izapolsk izapolsk commented Jul 7, 2017

  1. View.include passes incorrect parent to included widgets View.include passes incorrect parent to included widgets #40
  2. Included widgets aren't available in inherited views inherited views don't see included widgets #41

@mfalesni
Copy link
Contributor

@izapolsk That behaviour is intentional, including is not designed to supplement complex logic inside the class being included.

@mfalesni
Copy link
Contributor

@izapolsk Actually, it is described in readme, why it is done like that :)

@izapolsk
Copy link
Contributor Author

@mfalesni , I apparently missed that in readme. Actually, I even didn't think it was intentionally done because everywhere else in widgetastic widget names are exposed in inherited classes.

@mfalesni
Copy link
Contributor

I am okay with the inheritance thing, but the parent setting - I believe that needs to be done as a switch.

@izapolsk
Copy link
Contributor Author

@mfalesni , awesome. regarding parent setting, it relevant to only included widgets. You pass there view's parent instead of current view. So, current view cannot be accessed by included widgets as a result.
Please give me more info what isn't ok.

@mfalesni
Copy link
Contributor

@izapolsk Yep, the including view cannot be accesse currentlyd, there are two main reasons why I did that:

  1. Widgets can specify a root locator, that can fence the lookup even though the included thing may be completely independent.
  2. I don't think that the included widgets should necessary reach out and rely on specific widgets being in somewhere.

I am not a big fan of having the parent as the including widget, but I don't necessary oppose it, so I believe the .include(View, use_parent=True) would be a good use of that.

assert isinstance(some_form.input1.parent.parent, type(browser))
some_form2 = TestForm6(browser)
assert isinstance(some_form2.input1.parent.parent, TestForm6)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add some widgets into the forms 4 and 6 and assert widget ordering.

@mfalesni
Copy link
Contributor

@izapolsk I commented on the unit test, make sure the build passes and add coverage on the inherited widget including ordering. Could you also please modify the readme to include the mention of use_parent? Rephrase the article that there are two possibilities etc ...

@izapolsk
Copy link
Contributor Author

@mfalesni, please check whether this is what you expected to see.

@mfalesni mfalesni merged commit 91e4ae8 into RedHatQE:master Jul 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants