Skip to content

Local method/properties can be accessed by other classes #209

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

Open
kumo01GitHub opened this issue May 1, 2025 · 6 comments · May be fixed by #211
Open

Local method/properties can be accessed by other classes #209

kumo01GitHub opened this issue May 1, 2025 · 6 comments · May be fixed by #211

Comments

@kumo01GitHub
Copy link
Contributor

Feature Request

Motivation Behind Feature

On Android implementation, local methods and properties are defined as public. But I don't think these are expected to call by other methods.

Feature Description

  • Change local methods called by execute() accessibility from public to private.
  • change property uuid accessibility from public to private.

Alternatives or Workarounds

N/A

@kumo01GitHub
Copy link
Contributor Author

I tried to fix this issue with my branch and I made sure to resolve issue. If there are no problems, I would like to make PR.

@breautek
Copy link
Contributor

breautek commented May 1, 2025

I'm not so sure if this a good idea.

I would definitely be supportive of making these methods private if this was a new package but giving that this is a really old plugin that's always had public members, there could be systems that depends on these.

A third-party plugin (or even native workflow app) could fetch the plugin instance from PluginManager and use the public APIs directly.

While that definitely wouldn't be a common practice, I'm not sure if value of making these methods/properties private for proper encapsulation outweighs the risk of breaking third-party code that may depend on the public API.

@kumo01GitHub
Copy link
Contributor Author

While that definitely wouldn't be a common practice, I'm not sure if value of making these methods/properties private for proper encapsulation outweighs the risk of breaking third-party code that may depend on the public API.

I don't have certain answer for that. And I doubt that it's worth to change accessibility RIGHT NOW. So, how about add deprecated annotation and explanations to public methods?

@breautek
Copy link
Contributor

breautek commented May 2, 2025

That might be the best path forward, since it will be easy to remove the deprecation if we happen to get push-back later.

We can revisit this when preparing a major release.

@kumo01GitHub kumo01GitHub linked a pull request May 3, 2025 that will close this issue
5 tasks
@kumo01GitHub kumo01GitHub changed the title Local method/properties. can access by other classes Local method/properties can be access by other classes May 5, 2025
@kumo01GitHub kumo01GitHub changed the title Local method/properties can be access by other classes Local method/properties can be accessed by other classes May 5, 2025
@jcesarmobile
Copy link
Member

Plugins can add other plugins as dependencies, and subclass or use existing methods from the other plugins, this would break that use case.
For uuid might make sense since it's a property and there is a getUuid() method that is used to set the uuid value, but I see no point on making the methods private.
But the fact that uuid is static makes me think it's used somewhere.

@kumo01GitHub
Copy link
Contributor Author

kumo01GitHub commented May 9, 2025

Plugins can add other plugins as dependencies, and subclass or use existing methods from the other plugins, this would break that use case.

I agree generally. But there is the comment LOCAL METHODS at L83 and I interpreted local equals to private. That's why I don't think the methods are expected to be used by others in this case.

And also I think there are no contradictions when the reason for uuid is defined as static is device uuid provided by this plugin should be unique even if there are multiple instances.

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 a pull request may close this issue.

3 participants