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

Remove not used methods #4980

Closed
wants to merge 1 commit into from
Closed

Conversation

yui-knk
Copy link
Contributor

@yui-knk yui-knk commented Jan 16, 2018

No description provided.

@kares
Copy link
Member

kares commented Jan 16, 2018

RubyInstanceConfig is sensitive API, it might need to stay and go @Deprecated instead, right @enebo ?

@enebo
Copy link
Member

enebo commented Jan 16, 2018

@kares @yui-knk yeah this one I don't think we should remove or annotate as a deprecation. In this case, an extension author might want to know if ruby is from a command line. We don't want them to be calling FILE for this. These decisions are a little difficult YAGNI OTOH it is reasonable information in a consistent location someone might actually use?

The larger issue is we do not have an explicit native extension API (beyond ScriptingContainer which arguably is more of an embedding API) so I could see extension authors expecting this class to remain unchanged.

@enebo enebo closed this Jan 16, 2018
@kares kares added this to the Invalid or Duplicate milestone Jan 16, 2018
@yui-knk yui-knk deleted the remove_hasInlineScript branch January 17, 2018 00:24
@yui-knk
Copy link
Contributor Author

yui-knk commented Jan 17, 2018

@enebo
Sorry, I didn't explain it clearly enough.
RubyInstanceConfig has public boolean hasInlineScript() and public boolean isInlineScript(). Both of them return hasInlineScript , so I think we can omit (or deprecate) one of them.

@enebo
Copy link
Member

enebo commented Jan 17, 2018

@yui-knk oh ok yeah I agree then one could be deprecated. My logic would be to keep hasInlineScript() since it implies it was invoked as inline script but it may loads lots of other Ruby. Anyone else have an opinion?

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