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

Add toLocaleString() method to array #47

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

xanaDev
Copy link

@xanaDev xanaDev commented Oct 16, 2022

Description

Add a new method to the Arr class which returns a string representing the elements of the array, uses a default locale or user-provided locale.

  • Implement method
  • Write phpdoc for the function
  • Write test in tests/ArrTest.php
  • Execute composer format
  • Execute composer test-coverage
  • Check if your method has full test coverage
  • Add method in changelog
  • Update doc/arr.md adding a use case of the new method
  • Update cheatsheet (examples/cheatsheet.php)

Fixes issue #15

@xanaDev xanaDev force-pushed the feat/arr-to-local-string branch 2 times, most recently from 5c2cf1e to 4dcda51 Compare October 16, 2022 18:54
@xanaDev
Copy link
Author

xanaDev commented Oct 16, 2022

@roberto-butti I went ahead and implemented the method, let me know if there is anything I need to improve.

@roberto-butti
Copy link
Contributor

thank you @xanaDev for the pull request. i',m going to check the PR from my mobile device, but i can try to merge it next weekend. is it ok for you?

@xanaDev
Copy link
Author

xanaDev commented Oct 17, 2022

@roberto-butti No problem.
As for the failing tests, the locale "fr_FR" is not installed in the container running the GitHub actions, I'll look into it later (maybe there is a better approach), but any feedback is welcome.

src/Arr.php Outdated Show resolved Hide resolved
src/Arr.php Outdated Show resolved Hide resolved
src/Arr.php Outdated Show resolved Hide resolved
@roberto-butti
Copy link
Contributor

toLocaleString()

Hi @xanaDev , my suggestion is: like the toLocaleString in JS, if the locale doesn't exist or is not installed, return the string in the default locale.

@xanaDev
Copy link
Author

xanaDev commented Oct 22, 2022

@RoadSigns @roberto-butti Thanks for the feedback.

* Returns a string representing the elements of the array
* @return string the string representation
*/
public function toLocaleString(string $locale = 'en_US', string $timezone = 'UTC'): string
Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, while this covers the Date.prototype.toLocaleString() there is also the Object.prototype.toLocaleString() and the Number.prototype.toLocalString() that can be converted from the Array.protoype.toLocalString().

Link to the other types of toLocalString() conversions
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toLocaleString#examples

Copy link
Author

Choose a reason for hiding this comment

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

I can definitely add support for the other methods, I'll update the current PR unless otherwise advised.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. if you want you can do it in this PR, or if you prefer you could close this and create another new PR with this refactor.
I mention "refactor" because this package is for Array so I would like to provide the toLocaleString for Arr (public method). The toLocaleString for other types (number and data) is for a clear code that is easier to test.

@roberto-butti
Copy link
Contributor

Ehi @xanaDev thanks for the PR. If you want, i can merge it in a new branch , so i can fix the issue for managing differences between the operating system (the implementation works fine with GNU/Linux systems but the tests fail with Windows:
https://github.com/Hi-Folks/array/actions/runs/3303363390/jobs/5452461267
What do you think ?

@xanaDev
Copy link
Author

xanaDev commented Nov 7, 2022

@roberto-butti Sounds good, I didn't test the implementation in windows so no surprise there. Maybe I can add the missing implementations for other types within the new branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants