Strip micro-version and add upper bound when adding package imports

[alshamams]


This commit resets the micro-version to ‘0’ while importing packages as part of quick fix, as it is not relevant.


Fixes: #401

[github-actions]

Test Results

     277 files  ±0       277 suites  ±0   55m 59s ⏱️ + 3m 21s
  3 496 tests ±0    3 460 ✔️ ±0    36 💤 ±0  0 ❌ ±0 
10 619 runs  ±0  10 517 ✔️ ±0  102 💤 ±0  0 ❌ ±0 

Results for commit 147e200. ± Comparison against base commit 1d27d86.

♻️ This comment has been updated with latest results.

[HannesWell]

Hi @alshamams thank you for working on this.
While this fixes the linked issue in its most basic form I think this is a good opportunity to fix a few other flaws in this quick-fix.

In general it would be good to also specify the upper bound of the import version range and to move the logic to a more general place, i.e. to ImportPackageObject.getVersion().
You can get such String using the following to assemble a suitable VersionRange from a org.osgi.framework.Version:

new VersionRange("[" + version.getMajor() + "." + version.getMinor() + "," + (version.getMajor() + 1) + "]").toString()

With that the version of the ImportPackageObject does not have to be set explicitly anymore.

Besides that, the quick-fix currently only adds a version if the Import-Package header already exists. This could be fixed by replacing.

if (header == null) {
	bundle.setHeader(Constants.IMPORT_PACKAGE, pkgId);
} else if (header instanceof ImportPackageHeader ipHeader) {

by

if (header == null) {
	header = bundle.getModel().getFactory().createHeader(Constants.IMPORT_PACKAGE, pkgId);
}
if (header instanceof ImportPackageHeader ipHeader) {

[alshamams]

Thanks @HannesWell for the quick review.

In general it would be good to also specify the upper bound of the import version range and to move the logic to a more general place

Thanks, this looks clean and elegant. However, modifying the getVersion API will have implications in regular import package flow. (Not the quick fix one, but while adding import packages from Dependencies tab). Is this what you intend ? Just wanted to make sure you are aware of this change before I push.

Besides that, the quick-fix currently only adds a version if the Import-Package header already exists. This could be fixed by replacing.

I was aware of it, and was planning to do it in a follow-up PR, given that it is unrelated to the issue at hand. I think that I will make the change here itself.

[HannesWell]

In general it would be good to also specify the upper bound of the import version range and to move the logic to a more general place

Thanks, this looks clean and elegant. However, modifying the getVersion API will have implications in regular import package flow. (Not the quick fix one, but while adding import packages from Dependencies tab). Is this what you intend ? Just wanted to make sure you are aware of this change before I push.

I have not yet checked every caller path of the code, but yes in general it is good practice to specifiy an upper bound if one specifies a lower to follow semantic versioning. If one really wants to have no upper-bound, it can still be removed later. But from my experience I currently add an upper bound in 90% of the time I use the quick-fix.

Besides that, the quick-fix currently only adds a version if the Import-Package header already exists. This could be fixed by replacing.

I was aware of it, and was planning to do it in a follow-up PR, given that it is unrelated to the issue at hand. I think that I will make the change here itself.

Yes that's fine from my POV, this change is small enough at the moment and it is related enough to mix it.

[alshamams]

Hi @HannesWell ,
I have addressed the changes, and squashed and rebased the commits while adding you as the co-author. Kindly let me know in case of any more edits.

[HannesWell]

@alshamams I just applied the suggestions locally and also added handling for the default empty version (0.0.0) in ImportPackageObject so that in that case not a version range like [0.0.0,1.0.0) is added. This case

The great thing about specifying the upper bound in ImportPackageObject.getVersion() is, that it is now also added if one select and adds a package via the Editor directly or through the add dependencies link in the Automated Management of Dependencies section.
This is IMHO a great improvement for PDE respectively its users.

The current state looks very good to me and is for me ready for submission.
Are you fine with this as well?

[HannesWell]

I'm merging this now.
Thank you again @alshamams.