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

Refactoring for checks on strings containing formatted numeric values #3164

Merged
merged 3 commits into from
Nov 11, 2022

Conversation

MarkBaker
Copy link
Member

@MarkBaker MarkBaker commented Nov 10, 2022

This is:

- [ ] a bugfix
- [ ] a new feature
- [X] refactoring
- [ ] additional unit tests

Checklist:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • New unit tests have been added
  • Code style is respected
  • Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
  • CHANGELOG.md contains a short summary of the change
  • Documentation is updated as necessary

Why this change is needed?

Refactoring for checks on strings containing formatted numeric values (e.g. percentages and fractions) when used in mathematical operations in the Calculation Engine

This is a refactoring exercise to make extract the logic into its own class, making it easier to manipulate formatted string values into their numeric value, and adding handling for new formatting methods in the future

@oleibman
Copy link
Collaborator

@MarkBaker I see you've finally eliminated Php7.3. So why does Github still show a Php7.3 check and mark it required even though it will never run (and we therefore don't see "all required statuses have passed")? And, in a similar vein, should the Php8.2 test, which is no longer experimental, be marked as required?

@MarkBaker
Copy link
Member Author

@MarkBaker I see you've finally eliminated Php7.3. So why does Github still show a Php7.3 check and mark it required even though it will never run (and we therefore don't see "all required statuses have passed")? And, in a similar vein, should the Php8.2 test, which is no longer experimental, be marked as required?

All the 7.3 references are removed from the github side: I think that 7.3 reference is from scrutinizer. I don't have access to scrutinizer itself, that would be @PowerKiKi to check on that side.

The 8.2 tests are set for no failure allowed. Currently, the nightly build (set for allow failure) is still against the 8.2-dev branch of PHP, so the nightly is a duplication of 8.2; but that will change in two weeks when 8.2 goes to GA, and the nightly will switch to the planned 8.3 release.

@MarkBaker MarkBaker force-pushed the CalcEngine-Refactor_Formatted-Numbers branch from 2efb642 to 0ba0da2 Compare November 11, 2022 10:53
@MarkBaker MarkBaker force-pushed the CalcEngine-Refactor_Formatted-Numbers branch from 0ba0da2 to 6a50973 Compare November 11, 2022 11:28
@MarkBaker MarkBaker merged commit c928bfd into master Nov 11, 2022
@MarkBaker MarkBaker deleted the CalcEngine-Refactor_Formatted-Numbers branch November 25, 2022 13:59
@PowerKiKi
Copy link
Member

@MarkBaker I gave you access to Scrutinizer.

@oleibman I cannot see the same thing you saw anymore, but I am pretty sure that GitHub was still waiting for a CI to pass for PHP 7.3 because it previously existed. That's how GitHub Actions works, it expect the previous check to still exist and pass. It's a bit cumbersome when we remove checks, but it should auto-correct itself once we merge something with the missing check. I'll see what happens with #3223 ...

@oleibman
Copy link
Collaborator

oleibman commented Dec 2, 2022

@PowerKiKi Thank you for the explanation. I have confirmed with a new PR, and merging code with an existing PR, that both function correctly now.

MarkBaker added a commit that referenced this pull request Dec 21, 2022
### Added

- Extended flag options for the Reader `load()` and Writer `save()` methods
- Apply Row/Column limits (1048576 and XFD) in ReferenceHelper [PR #3213](#3213)
- Allow the creation of In-Memory Drawings from a string of binary image data, or from a stream. [PR #3157](#3157)
- Xlsx Reader support for Pivot Tables [PR #2829](#2829)
- Permit Date/Time Entered on Spreadsheet to be calculated as Float [Issue #1416](#1416) [PR #3121](#3121)

### Changed

- Nothing

### Deprecated

- Direct update of Calculation::suppressFormulaErrors is replaced with setter.
- Font public static variable defaultColumnWidths replaced with constant DEFAULT_COLUMN_WIDTHS.
- ExcelError public static variable errorCodes replaced with constant ERROR_CODES.
- NumberFormat constant FORMAT_DATE_YYYYMMDD2 replaced with existing identical FORMAT_DATE_YYYYMMDD.

### Removed

- Nothing

### Fixed

- Fixed handling for `_xlws` prefixed functions from Office365 [Issue #3245](#3245) [PR #3247](#3247)
- Conditionals formatting rules applied to rows/columns are removed [Issue #3184](#3184) [PR #3213](#3213)
- Treat strings containing currency or accounting values as floats in Calculation Engine operations [Issue #3165](#3165) [PR #3189](#3189)
- Treat strings containing percentage values as floats in Calculation Engine operations [Issue #3155](#3155) [PR #3156](#3156) and [PR #3164](#3164)
- Xlsx Reader Accept Palette of Fewer than 64 Colors [Issue #3093](#3093) [PR #3096](#3096)
- Use Locale-Independent Float Conversion for Xlsx Writer Custom Property [Issue #3095](#3095) [PR #3099](#3099)
- Allow setting AutoFilter range on a single cell or row [Issue #3102](#3102) [PR #3111](#3111)
- Xlsx Reader External Data Validations Flag Missing [Issue #2677](#2677) [PR #3078](#3078)
- Reduces extra memory usage on `__destruct()` calls [PR #3092](#3092)
- Additional properties for Trendlines [Issue #3011](#3011) [PR #3028](#3028)
- Calculation suppressFormulaErrors fix [Issue #1531](#1531) [PR #3092](#3092)
- Permit Date/Time Entered on Spreadsheet to be Calculated as Float [Issue #1416](#1416) [PR #3121](#3121)
- Incorrect Handling of Data Validation Formula Containing Ampersand [Issue #3145](#3145) [PR #3146](#3146)
- Xlsx Namespace Handling of Drawings, RowAndColumnAttributes, MergeCells [Issue #3138](#3138) [PR #3136](#3137)
- Generation3 Copy With Image in Footer [Issue #3126](#3126) [PR #3140](#3140)
- MATCH Function Problems with Int/Float Compare and Wildcards [Issue #3141](#3141) [PR #3142](#3142)
- Fix ODS Read Filter on number-columns-repeated cell [Issue #3148](#3148) [PR #3149](#3149)
- Problems Formatting Very Small and Very Large Numbers [Issue #3128](#3128) [PR #3152](#3152)
- XlsxWrite preserve line styles for y-axis, not just x-axis [PR #3163](#3163)
- Xlsx Namespace Handling of Drawings, RowAndColumnAttributes, MergeCells [Issue #3138](#3138) [PR #3137](#3137)
- More Detail for Cyclic Error Messages [Issue #3169](#3169) [PR #3170](#3170)
- Improved Documentation for Deprecations - many PRs [Issue #3162](#3162)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants