-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
task done #2198
base: master
Are you sure you want to change the base?
task done #2198
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your hard work on the formatDate
function! 😊 While your implementation meets many of the task requirements, there are a couple of critical issues that need to be addressed to ensure the function works correctly.
-
Loop Iteration: The loop should iterate over the entire
toParts
array. Please change the condition toi < toParts.length
to ensure all parts of the date are included in thedateTo
array. -
Separator Handling: The
findSeparator
function is incorrectly applied totoFormat
, which is an array. This function expects a string. You should determine the separator for the output format differently, perhaps by using a predefined separator or ensuringtoFormat
is a string.
Once these issues are resolved, your code will be in great shape! Keep up the great work, and don't hesitate to reach out if you have any questions. 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review
button in the Reviewers tab.
src/formatDate.js
Outdated
const [fromParts] = [fromFormat.slice()]; | ||
const separators = ['-', '/', ' ', '.']; | ||
const separator = findSeparator(date, separators); | ||
const newSeparator = findSeparator(toFormat, separators); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The findSeparator
function is incorrectly used here. toFormat
is an array, but findSeparator
expects a string. You should determine the separator for the output format differently, possibly by using a predefined separator or by ensuring toFormat
is a string.
src/formatDate.js
Outdated
} | ||
} | ||
|
||
for (let i = 0; i < toParts.length - 1; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop should iterate over the entire toParts
array. Change the condition to i < toParts.length
to ensure all parts of the date are included in the dateTo
array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
Great job on improving the formatDate
function! 🎉 Your code now meets the task requirements and constraints, effectively handling date format conversions, including changing separators, reordering date parts, and converting years between YY
and YYYY
formats. Here are some key points from the review:
-
Positive Changes:
- The loop now correctly iterates over the
toParts
array, ensuring all parts of the date are included in thedateTo
array. - The separator handling has been improved by correctly extracting the separator from the
toFormat
array.
- The loop now correctly iterates over the
-
Areas for Improvement:
- The
newSeparator
is incorrectly derived from thetoFormat
array. The last element oftoFormat
is being used as a separator, which may not be correct. Consider using a predefined separator or extracting it from a correctly formatted string.
- The
While the code is approved to help you move forward with your learning, please review the comments and address the separator issue to enhance the functionality of your code. Keep up the good work, and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
const [toParts, newSeparator] = [ | ||
toFormat.slice(0, -1), | ||
toFormat[toFormat.length - 1], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newSeparator
is incorrectly derived from the toFormat
array. The last element of toFormat
is being used as a separator, which may not be correct. Consider using a predefined separator or extracting it from a correctly formatted string.
No description provided.