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

Array CRUDs improvements (part #1) #708

Merged
merged 81 commits into from Jul 5, 2021
Merged

Conversation

voodoo11
Copy link
Collaborator

@voodoo11 voodoo11 commented Jun 12, 2021

Context

This is a first part of improvements of CRUD operations with array formulas.

  1. Transpose no longer needs to be limited to a numeric type.
  2. Allow to add row across array. Array size won't be affected.
  3. Allow to remove row across array. Array size won't be affected.
  4. Allow to change content inside array. SPILL will be returned if changing value other that left corner

Splitted, as it is already huge PR. Should be merged with second part (#732).

How has this been tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature or improvement (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Additional language file or change to the existing one (translations)

Related issue(s):

Checklist:

  • My code follows the code style of this project,
  • My change requires a change to the documentation,
  • I described the modification in the CHANGELOG.md file.

@voodoo11 voodoo11 marked this pull request as ready for review June 26, 2021 11:55
@@ -151,6 +160,11 @@ export class MatrixVertex extends FormulaVertex {
}
}

setNoSpace(): InterpreterValue {
this.matrix = new ErroredMatrix(new CellError(ErrorType.REF, ErrorMessage.NoSpaceForArrayResult), MatrixSize.error())
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a SPILL error type?
obraz

Suggested change
this.matrix = new ErroredMatrix(new CellError(ErrorType.REF, ErrorMessage.NoSpaceForArrayResult), MatrixSize.error())
this.matrix = new ErroredMatrix(new CellError(ErrorType.SPILL, ErrorMessage.NoSpaceForArrayResult), MatrixSize.error())

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we provide some additional info to the error, as width and height that would allow to display it as shown on the screenshot?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shouldn't this be a SPILL error type?

GS would return REF and that's what I followed. Do we want to introduce new error type at this moment?

Can we provide some additional info to the error, as width and height that would allow to display it as shown on the screenshot?

It would be a nice feature indeed but right now we do not store any additional data inside errors except address of a root of a problem. I would save it for the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a SPILL error type?

GS would return REF and that's what I followed. Do we want to introduce new error type at this moment?

Yes, please. It will be an additional information that helps to differentiate them. ODFF allows vendors to introduce custom errors.

Can we provide some additional info to the error, as width and height that would allow to display it as shown on the screenshot?

It would be a nice feature indeed but right now we do not store any additional data inside errors except address of a root of a problem. I would save it for the future.

Ok 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

src/HyperFormula.ts Show resolved Hide resolved
test/cruds/adding-columns.spec.ts Show resolved Hide resolved
test/cruds/adding-row.spec.ts Show resolved Hide resolved
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 2, 2021

This pull request introduces 1 alert when merging b5b2318 into cc23e19 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 2, 2021

This pull request introduces 1 alert when merging c027918 into cc23e19 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 2, 2021

This pull request introduces 1 alert when merging 92a04a2 into cc23e19 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

Copy link
Contributor

@wojciechczerniak wojciechczerniak left a comment

Choose a reason for hiding this comment

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

Awesome, thank you 🏆

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 5, 2021

This pull request introduces 1 alert and fixes 2 when merging 878265a into 8914f4a - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

fixed alerts:

  • 2 for Unused variable, import, function or class

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 5, 2021

This pull request fixes 2 alerts when merging a58b89f into 8914f4a - view on LGTM.com

fixed alerts:

  • 2 for Unused variable, import, function or class

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 5, 2021

This pull request fixes 2 alerts when merging 0a3ca63 into 880126e - view on LGTM.com

fixed alerts:

  • 2 for Unused variable, import, function or class

@voodoo11 voodoo11 merged commit 72862d7 into develop Jul 5, 2021
@voodoo11 voodoo11 deleted the jk/matrices-take-2-cruds branch July 5, 2021 09:03
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.

None yet

3 participants