ChakraCore icon indicating copy to clipboard operation
ChakraCore copied to clipboard

Fix issue 5442

Open Nazeeh21 opened this issue 3 years ago • 8 comments

This PR proposes the fix for issue #5442. @rhuanjl

Nazeeh21 avatar Sep 08 '21 14:09 Nazeeh21

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.

ppenzin avatar Sep 10 '21 23:09 ppenzin

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.

Okay! I'll get it done.

Nazeeh21 avatar Sep 11 '21 05:09 Nazeeh21

Thanks @ppenzin I'll take a look when there's a test case.

rhuanjl avatar Sep 14 '21 19:09 rhuanjl

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.

Nazeeh21 avatar Sep 15 '21 05:09 Nazeeh21

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:

  1. (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.
  2. 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 in DateCtr.js which does its tests a different way and doesn't use a baseline file
  3. 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)

rhuanjl avatar Sep 18 '21 22:09 rhuanjl

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.

Nazeeh21 avatar Sep 19 '21 09:09 Nazeeh21

I have made the suggested changes. Do let me know if any of the changes are required.

Nazeeh21 avatar Sep 21 '21 06:09 Nazeeh21

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?

rhuanjl avatar Sep 24 '21 21:09 rhuanjl