Skip to content
This repository has been archived by the owner on Apr 21, 2023. It is now read-only.

[378980] Make line delimiter for Manifest configurable by CodeConfig #96

Merged
merged 1 commit into from Sep 12, 2016

Conversation

kthoms
Copy link

@kthoms kthoms commented Sep 12, 2016

Line delimiter is determined by CodeConfig and reached into MergeableManifest.

Signed-off-by: Karsten Thoms karsten.thoms@itemis.de

@eclipsewebmaster
Copy link

Issue tracker reference:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=378980

@meysholdt
Copy link
Contributor

\r\n is hard-coded in java.util.jar.Manifest.write(OutputStream), even though the spec allows for \r\n and \n

@kthoms
Copy link
Author

kthoms commented Sep 12, 2016

Right, I was not aware of that. So does this mean since standard Java Manifest uses \r\n it is also mandatory for MergeableManifest to do the same? Seems so, i.e. I should close the bug as WONTFIX?

@meysholdt
Copy link
Contributor

Further investigation shows that java.util.jar.Manifest.read(InputStream) supports both \r\n and \n. Also, the Eclipse Manifest Editor supports both \r\n and \n\. Furthermore, the MANIFEST.MFs in xtext-core only contain \n. So I'm fine with making line-edings configurable in MergeableManifest.

Can you confirm that your change doesn't mix line-edings, e.g. because a method from the super-class (Manifest) adds \r\n but in MergeableManifest \n is configured?

@kthoms
Copy link
Author

kthoms commented Sep 12, 2016

screenshot 5

This is how it looks like after generating. The example above is without any configuration, it will use Strings.newLine() by default which is "\n". You can see all line endings are such.

The second example shows explicit configuration of "\r\n". Again, all line endings are the same.

@@ -50,38 +50,47 @@
public static final Attributes.Name BUNDLE_LOCALIZATION = new Attributes.Name("Bundle-Localization");
public static final Attributes.Name BUNDLE_ACTIVATOR = new Attributes.Name("Bundle-Activator");

private static final String LINEBREAK = "\r\n";
private static final String LINEBREAK = Strings.newLine();
Copy link
Contributor

Choose a reason for hiding this comment

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

this filed is only used by deprecated methods, right? Then it should be deprecated itself.

Copy link
Author

Choose a reason for hiding this comment

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

Correct, it is only used in deprecated contexts. Will fix that.

Signed-off-by: Karsten Thoms <karsten.thoms@itemis.de>
@meysholdt
Copy link
Contributor

thx!

@meysholdt meysholdt merged commit 86dddf8 into eclipse:master Sep 12, 2016
@kthoms
Copy link
Author

kthoms commented Sep 12, 2016

Thanks for the review :)

@kthoms kthoms deleted the kth/bug378980 branch September 12, 2016 12:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants