ChakraCore
ChakraCore copied to clipboard
Fix issue 5442
This PR proposes the fix for issue #5442. @rhuanjl
Thank you for the PR! The change itself looks good to me so far (though I'd like to hear from @rhuanjl), but there are a couple bits left.
Can you please add a test? You can edit Conversions.js
and Conversions.baseline
in test/Date
directory. It is also possible to add a separate file, but I think this way is easier. Let me know if you need help with this.
Can you also please sign the contribution agreement, by adding yourself to ContributionAgreement.md
(in the top level directory) and update the copyright in the file you have changed and the test to include a second copyright line:
// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
right below the Microsoft one.
After that CI should pass.
Thank you for the PR! The change itself looks good to me so far (though I'd like to hear from @rhuanjl), but there are a couple bits left.
Can you please add a test? You can edit
Conversions.js
andConversions.baseline
intest/Date
directory. It is also possible to add a separate file, but I think this way is easier. Let me know if you need help with this.Can you also please sign the contribution agreement, by adding yourself to
ContributionAgreement.md
(in the top level directory) and update the copyright in the file you have changed and the test to include a second copyright line:// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
right below the Microsoft one.
After that CI should pass.
Okay! I'll get it done.
Thanks @ppenzin I'll take a look when there's a test case.
I have added a test in Conversions.js
and also added myself in ContributionAgreement.md
. Please do let me know about any of the improvements or corrections.
Hi @Nazeeh21 thank you for this contribution and sorry for the slow reply I've had a particularly busy week.
The fix looks right though a couple of things to update:
- (minor) please could you format your if statement the same way as others in the file it's in (we like to keep code style consistent where we can.
- Your new test failed (see the CI fails for MSVC x64.Test and MSVC x86.Test) - this is because that testfile
Conversions.js
is generating a text output that is compared against a separate baseline file and you didn't update the baseline. To make it easier and not have to update two files please can you instead put the new test inDateCtr.js
which does its tests a different way and doesn't use a baseline file - The CI Style check failed - this checks that we have updated the copyright text at the top of all files we update, this is just a case of updating the banner text of any file you've edited to say:
//-------------------------------------------------------------------------------------------------------
// Copyright (C) Microsoft. All rights reserved.
// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
//-------------------------------------------------------------------------------------------------------
(the original work is Microsoft's - we add the extra copyright line to cover work done since MS stopped developing CC)
That is completely alright and not an issue.
Coming to the code changes I'll format the if
statement as required, move the new test to DateCtr.js
and also update the banner of all the changed files.
I have made the suggested changes. Do let me know if any of the changes are required.
The CI tests have failed - I think your test update isn't exactly right please can it have another look at the test file and check if you need to add a result as well as a test?