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

Issue #19 non-public functionality #84

Closed
wants to merge 4 commits into from

Conversation

crookse
Copy link
Member

@crookse crookse commented Oct 23, 2020

Fixes #19

Description

  • Adds getNonPublicPropertyValue()
  • Adds invokeNonPublicMethod()
  • Adds tests to test the above new methods

Copy link
Member

@ebebbington ebebbington left a comment

Choose a reason for hiding this comment

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

Nice one!

@Guergeiro
Copy link
Member

I'm against this change. There's a reason why private/public/protected signatures exist. Making this kind of change will violate encapsulation in OOP.
If something needs to be accessed it should be either by using Getters/Setters or making the attributes public. Using the latter is the "lazy" way, btw.

I would find the specific points trying to access and provide Getters for them.

@crookse
Copy link
Member Author

crookse commented Oct 26, 2020

@Guergeiro, so you don't see a use case to test non public methods?

@ebebbington
Copy link
Member

I would have to disagree with @Guergeiro, it's not as if private/protected methods are accessible all the time - you'd have to directly explicitly use rhum to do this - plus, we can't count on other API's having getters and setters.

@Guergeiro
Copy link
Member

@Guergeiro, so you don't see a use case to test non public methods?

Testing private methods should be done with Reflection and not with a method that changes the class itself.

@crookse
Copy link
Member Author

crookse commented Oct 26, 2020

so should we mimic a reflection API?

@Guergeiro
Copy link
Member

I would have to disagree with @Guergeiro, it's not as if private/protected methods are accessible all the time - you'd have to directly explicitly use rhum to do this - plus, we can't count on other API's having getters and setters.

That method is an API for the end user... That's the issue. An API shouldn't expose everything to the user, specially something which is related to the framework internal behaviour and not useful in anyway besides framework maintainers.

@ebebbington
Copy link
Member

Yeah i guess i see your point - i mean we take that approach with drashland modules already (or any code)

@crookse
Copy link
Member Author

crookse commented Oct 29, 2020

Breno makes a good point. we will add documentation on how to reflect classes to test non-public members, but we will not be exposing an API to do it for users.

@crookse crookse closed this Oct 29, 2020
@crookse crookse deleted the issue-#19-reflection-functionality branch October 31, 2020 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants