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

Kept order of headers #32

Merged
merged 1 commit into from
May 25, 2018
Merged

Kept order of headers #32

merged 1 commit into from
May 25, 2018

Conversation

tearwyx
Copy link
Contributor

@tearwyx tearwyx commented May 23, 2018

When the pofile loads and saves the PO file saved by another application, headers will be swapped. This then causes frequent differences in commits when PO file is indexed by git. A possible solution would be to change an object containing headers to an array.

@rubenv
Copy link
Owner

rubenv commented May 23, 2018

Hmmm, this would mean a major API break.

@tearwyx
Copy link
Contributor Author

tearwyx commented May 23, 2018

Or save the order of headers to the new property for serialization.

@rubenv
Copy link
Owner

rubenv commented May 23, 2018

Or save the order of headers to the new property for serialization.

Could you elaborate a bit on that?

@tearwyx
Copy link
Contributor Author

tearwyx commented May 23, 2018

The new instance property of type Array will contain header names. Their order will be taken from parsing. Headers will be serialized in this order.

@rubenv
Copy link
Owner

rubenv commented May 23, 2018

So the existing headers property can stay as it is, with a new headerOrder (or something like that) array for the serialization order, right?

That sounds like a better (backwards-compatible) solution. Should make the change much smaller as well.

@tearwyx
Copy link
Contributor Author

tearwyx commented May 23, 2018

The problem is how to sort the default headers. Are the default headers necessary?

@tearwyx tearwyx force-pushed the order-of-headers branch 4 times, most recently from 08486e3 to c7a6038 Compare May 24, 2018 10:34
@tearwyx
Copy link
Contributor Author

tearwyx commented May 24, 2018

@rubenv, headerOrder implemented.

lib/po.js Outdated
'Content-Type': '',
'Content-Transfer-Encoding': '',
'Plural-Forms': '',
};
Copy link
Owner

Choose a reason for hiding this comment

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

Can we keep this? I know it's ugly, but (if I remember correctly) some tools balk when these are missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kept.

lib/po.js Outdated
var self = this;
keys.forEach(function (key) {
this.headerOrder.forEach(function (key) {
Copy link
Owner

Choose a reason for hiding this comment

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

What happens if I manually do po.headers["test"] = "123".

Not sure if that will still get serialized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@rubenv
Copy link
Owner

rubenv commented May 24, 2018

Great, thanks for the fixes! Really like it now, as it's now a non-breaking change. I see no reason not to include this.

Can we come up with a unit test that fails without this in place (but obviously works with)? That way we'll be assured that we won't break it in the future.

Such a test might depend on the enumeration order in JS (not 100% sure how that works), so might be hard to write. Don't worry if that's the case, we'll just merge without one then.

@tearwyx
Copy link
Contributor Author

tearwyx commented May 25, 2018

@rubenv, some unit test added.

@rubenv rubenv merged commit 8bd7810 into rubenv:master May 25, 2018
@rubenv
Copy link
Owner

rubenv commented May 25, 2018

Perfect! Excellent work. Merged!

@rubenv
Copy link
Owner

rubenv commented May 25, 2018

Also pushed to NPM as v1.0.11. Thanks a lot!

@tearwyx
Copy link
Contributor Author

tearwyx commented May 25, 2018

@rubenv, thank you too.

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.

2 participants