Add ManifestElement.parseBundleManifest without map parameter

[laeubi]

Currently the existing ManifestElement.parseBundleManifest with a wild mixture of parameters, some using access restricted maps, some use null some use a hashmap (what likely causes issue if case of headers is different than expected).

This adds a new method that simply removes the map parameter and uses a CaseInsensitiveDictionaryMap that is also used internally in the framework and is most likely the most natural choice for this usecase.

[github-actions]

Test Results

  660 files  ±0    660 suites  ±0   1h 11m 26s ⏱️ -58s
2 195 tests ±0  2 148 ✅ ±0   47 💤 ±0  0 ❌ ±0 
6 729 runs  ±0  6 586 ✅ ±0  143 💤 ±0  0 ❌ ±0 

Results for commit 3eef0d4. ± Comparison against base commit 50e03e5.

♻️ This comment has been updated with latest results.

[tjwatson]

The package version org.eclipse.osgi.util will need to increase to 1.2 for the new API. This seems like overkill on convenience to me when one can simply use the existing method and choose the Map they want:

  parseBundleManifest(manifest, new TreeMap<>(String::compareToIgnoreCase));

Yet simply adds to the overall API we have to support forever.

[laeubi]

This seems like overkill on convenience to me when one can simply use the existing method and choose the Map they want

Convenience matters (for me) here especially as it is the most common case, incrementing a litte number in the manifest looks like not to much effort for that, every-time I have use for that method I need to think about what will happen if I pass null, then I notice it is a plain HashMap (== case sensitive keys) what is not what I want when working with manifest in 99% of the time, then I see equinox uses CaseInsensitiveDictionaryMap (but that's in a discouraged package) so next I need to find out how to get a plain java variant (is there one), then finally stumbles over the TreeMap (whats not that obvious) and so on...

Yet simply adds to the overall API we have to support forever.

As it just delegates to the existing method we have to support forever I really don't see much effort here on the long run :-)

[merks]

It seems strange that it currently creates a HashMap when null is supplied. That's kind of useless and is not well documented so it could arguably be changed to any Map implementation, e.g., new TreeMap<>(String::compareToIgnoreCase). That might break someone, but at least it could be documented to do that rather than just create a Map, "guess which type".

One could also argue that (all?) confusion could be eliminated with better documentation instead of a convenience API method.

[laeubi]

The null behavior is already documented, we already discussed that in the OSGi spec call yesterday I just find such method with "optional" parameters and I don't see how adding a new (documented) method would harm more than changing the existing implementation with the risk that someone (whyever) has relied on the HashMap returned.

[laeubi]

@tjwatson can we have an agreement here? Would you be fine now we incremented package version and just think its maybe not worth the efforts or do you have general concerns and we should dispose this all together.

[laeubi]

@tjwatson @merks I would assume further silence means agreement that this PR is okay to merge even though you personally might think it is not absolutely required!?