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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - DateTimeFormat helpers #2064

Closed
wants to merge 5 commits into from
Closed

[Merged by Bors] - DateTimeFormat helpers #2064

wants to merge 5 commits into from

Conversation

NorbertGarfield
Copy link
Contributor

@NorbertGarfield NorbertGarfield commented May 7, 2022 •
edited
Loading

This Pull Request adds several helpers to resolve #1562.

It adds the following subroutines:

  • toDateTimeOptions
  • GetOption
  • GetNumberOption
  • DefaultNumberOption

@codecov
Copy link

codecov bot commented May 7, 2022 •
edited
Loading

Codecov Report

Merging #2064 (051d3c9) into main (9d89e12) will increase coverage by 0.11%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##             main    #2064      +/-   ##
==========================================
+ Coverage   43.99%   44.10%   +0.11%     
==========================================
  Files         215      215              
  Lines       19327    19368      +41     
==========================================
+ Hits         8502     8542      +40     
- Misses      10825    10826       +1
Impacted Files Coverage Δ
boa_engine/src/builtins/intl/mod.rs 58.86% <94.11%> (+4.83%) ⬆️
boa_engine/src/builtins/intl/date_time_format.rs 56.25% <100.00%> (+43.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d89e12...051d3c9. Read the comment docs.

raskad
raskad requested changes May 8, 2022
View reviewed changes
Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

Thank you for taking this on! I did a in depth review and left some comments.

boa_engine/src/builtins/intl/date_time_format.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/date_time_format.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/date_time_format.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/date_time_format.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/date_time_format.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/mod.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/mod.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/mod.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/date_time_format.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/date_time_format.rs Outdated Show resolved Hide resolved
@NorbertGarfield NorbertGarfield force-pushed the garfield/date-time-style branch from 75e994b to 2676d0b Compare May 11, 2022 13:22
@NorbertGarfield NorbertGarfield force-pushed the garfield/date-time-style branch from 2676d0b to bd5287d Compare May 12, 2022 09:04
@NorbertGarfield NorbertGarfield requested a review from jedel1043 May 12, 2022 09:05
jedel1043
jedel1043 reviewed May 12, 2022
View reviewed changes
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Some additional comments that should improve the code :)

boa_engine/src/builtins/intl/date_time_format.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/date_time_format.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/date_time_format.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/date_time_format.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/date_time_format.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/mod.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/mod.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/mod.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/mod.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/tests.rs Outdated Show resolved Hide resolved
@NorbertGarfield NorbertGarfield requested a review from jedel1043 May 13, 2022 21:21
jedel1043
jedel1043 approved these changes May 13, 2022
View reviewed changes
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good!

raskad
raskad approved these changes May 13, 2022
View reviewed changes
Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

Nice work!

@raskad raskad added enhancement New feature or request builtins PRs and Issues related to builtins/intrinsics labels May 13, 2022
@raskad raskad added this to the v0.15.0 milestone May 13, 2022
@raskad
Copy link
Member

raskad commented May 13, 2022

bors r+

bors bot pushed a commit that referenced this pull request May 13, 2022
@NorbertGarfield
DateTimeFormat helpers (#2064)
5a0ff91 
This Pull Request adds several helpers to resolve #1562.

It adds the following subroutines:

- toDateTimeOptions
- GetOption
- GetNumberOption
- DefaultNumberOption
@bors
Copy link

bors bot commented May 13, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title DateTimeFormat helpers [Merged by Bors] - DateTimeFormat helpers May 13, 2022
@bors bors bot closed this May 13, 2022
Razican pushed a commit that referenced this pull request Jun 8, 2022
@NorbertGarfield @Razican
DateTimeFormat helpers (#2064)
a7d0385 
This Pull Request adds several helpers to resolve #1562.

It adds the following subroutines:

- toDateTimeOptions
- GetOption
- GetNumberOption
- DefaultNumberOption
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jedel1043 jedel1043 jedel1043 approved these changes

@raskad raskad raskad approved these changes

@Razican Razican Awaiting requested review from Razican

@RageKnify RageKnify Awaiting requested review from RageKnify

@jasonwilliams jasonwilliams Awaiting requested review from jasonwilliams

@HalidOdat HalidOdat Awaiting requested review from HalidOdat

Assignees
No one assigned
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request
Projects
None yet
Milestone
v0.15.0
Development

Successfully merging this pull request may close these issues.

Implement missing internationalization related methods
3 participants
@NorbertGarfield @raskad @jedel1043