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

Enhance de #442

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Enhance de #442

wants to merge 5 commits into from

Conversation

georgd
Copy link
Contributor

@georgd georgd commented Mar 10, 2022

No description provided.

});
});

// test("Test - Strict mode", function () {
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't leave the commented out code like this.
If we don't have DE strict mode, let's remove it.

});
});

//test("Test - Nested time ago", function () {
Copy link
Owner

Choose a reason for hiding this comment

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

Same as previous comment about not leaving commented code.

If we are not ready to support the nested time ref, lets remove this and add it later.

}

const unit = TIME_UNIT_DICTIONARY[match[4].toLowerCase()];
let timeUnits = {};
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: first, I'd rename num to value and I think let timeUnits = { unit: num }; is clearer to understand.

@wanasit
Copy link
Owner

wanasit commented Mar 28, 2022

Hey @georgd. Thanks for the improvement!
Could you please address or reply my comments?

@dpschen
Copy link

dpschen commented Mar 19, 2023

Hey @georgd, this is really useful! In case you currently don’t have time to continue on this would you be fine if I would pick up this PR?

@georgd
Copy link
Contributor Author

georgd commented Mar 19, 2023

Hi, indeed I sadly won't have time for this for another couple of months, as it seems. I'd be happy to hand this over to you 😀

@georgd
Copy link
Contributor Author

georgd commented Mar 19, 2023

Back then, I started refactoring the code to bring the DE part on par with EN but I am better with regex than with js so didn't get far in the little time I had. When I find the time, would you mind me proposing improvements?

@dpschen
Copy link

dpschen commented Mar 23, 2023

I'm not sure how much time I'll have. But probably it won't take a couple of months 😄

When I find the time, would you mind me proposing improvements?

Sure!

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.

3 participants