Skip to content

Remove unused declarations#848

Merged
milleruntime merged 6 commits intoapache:masterfrom
milleruntime:remove-unused-delcarations
Dec 20, 2018
Merged

Remove unused declarations#848
milleruntime merged 6 commits intoapache:masterfrom
milleruntime:remove-unused-delcarations

Conversation

@milleruntime
Copy link
Copy Markdown
Contributor

Second commit is questionable whether to remove a new unused parameter before released to API in 2.0. Other removals should all be internal.

Copy link
Copy Markdown
Member

@mikewalch mikewalch left a comment

Choose a reason for hiding this comment

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

I would just be very careful with API

@milleruntime
Copy link
Copy Markdown
Contributor Author

Reverted the commit that touched the new API. Then reran the code inspection to see what else is now unused.

@milleruntime
Copy link
Copy Markdown
Contributor Author

@mikewalch or @keith-turner you guys OK with the rest of the changes?

@milleruntime
Copy link
Copy Markdown
Contributor Author

@ctubbsii thanks for taking a look at this. This was more of an experiment using Intellij Run Inspection by name but it became too cumbersome with our API and pluggable interfaces. I plan on using a more focused approach for removing unused code next time.

@ctubbsii
Copy link
Copy Markdown
Member

@milleruntime I thought this worked out fine. It caught some stuff we maybe didn't want removed... but I think we caught all those in the reviews. Even if we didn't, the risk was minimal, since none of it touched the public API.

@ctubbsii
Copy link
Copy Markdown
Member

Eventually, all our "internal pluggable" stuff should be in declared SPI packages, to make it easier to ensure stability. We're still stuck with the historical stuff for now, though.

@milleruntime milleruntime merged commit d06c3fa into apache:master Dec 20, 2018
@milleruntime milleruntime deleted the remove-unused-delcarations branch December 20, 2018 23:13
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.

4 participants