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

Add ZipEntryStorage.getArchivePath() and deprecate its getArchive() #1545

Conversation

HannesWell
Copy link
Member

This should help to prevent issues like eclipse-platform/eclipse.platform.ui#2251.

What's your opinion regarding just deprecating ZipEntryStorage.getArchive() or deprecating it for removal?
We could keep it as deprecated as back-up for cases were another solution doesn't exist. But we could also use the deprecation time to work on alternatives if necessary.

The only caller in the SDK is in JDT and would be satisfied with the new method that could be applied with
eclipse-jdt/eclipse.jdt.debug#486

Copy link
Contributor

github-actions bot commented Sep 7, 2024

Test Results

 1 734 files  ±0   1 734 suites  ±0   1h 28m 55s ⏱️ - 4m 22s
 3 979 tests ±0   3 957 ✅ +1   22 💤 ±0  0 ❌  - 1 
12 534 runs  ±0  12 370 ✅ +3  164 💤 ±0  0 ❌  - 3 

Results for commit bc2b51a. ± Comparison against base commit b12f857.

♻️ This comment has been updated with latest results.

@vogella
Copy link
Contributor

vogella commented Sep 7, 2024

+1 for deprecate for removal

@HeikoKlare
Copy link
Contributor

If there is a (good) chance that the usage can be replaced in all existing clients, I don't see a good reason for not deprecating it for removal. It does not mean that is has to be removed, but only gives the chance to do so (in two years or so). And if a use case shows up that cannot be realized in any other way, the decision to keep the method as deprecated (and remove the "removal" flag) can also be made later, can't it?

@HannesWell HannesWell force-pushed the add-ZipEntryStorage-getArchivePath branch from b660a54 to c3db6bd Compare September 8, 2024 09:30
@HannesWell
Copy link
Member Author

It does not mean that is has to be removed, but only gives the chance to do so (in two years or so). And if a use case shows up that cannot be realized in any other way, the decision to keep the method as deprecated (and remove the "removal" flag) can also be made later, can't it?

Exactly. Just because an element is deprecated for removal doesn't mean it has to be removed eventually. The removal can always delayed (because a longer transition period is need or because it is forgotten etc.), the deprecation can be degraded to 'not-for-removal' to just discourage its use or can be revoked entirely. Since it cannot break clients if a method is kept this is not critical, just the removal is critical because it has the potential to break clients (and that's why the deprecation process exists).

If there is a (good) chance that the usage can be replaced in all existing clients, I don't see a good reason for not deprecating it for removal.

My problem is that I cannot tell anything about callers outside the SDK and I'm not so deeply familiar with this part of the code that I can say for sure that there is no chance that a caller for example looks up another entry in the archive.

But yes I'm also in favor in going the complete path to its end in the first step and deprecate the method for removal and if somebody has use-cases that cannot be implemented in other ways we can still adapt accordingly.

@HannesWell
Copy link
Member Author

If there is no objection I plan to submit this tomorrow (Tuesday) evening.

@HannesWell HannesWell force-pushed the add-ZipEntryStorage-getArchivePath branch from c3db6bd to bc2b51a Compare September 10, 2024 16:57
@HannesWell HannesWell merged commit 75d509d into eclipse-platform:master Sep 10, 2024
16 checks passed
@HannesWell HannesWell deleted the add-ZipEntryStorage-getArchivePath branch September 10, 2024 17:45
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.

3 participants