Skip to content

Pu/matrixinput - #671

Merged
izulin merged 18 commits into
developfrom
pu/matrixinput
May 27, 2021
Merged

Pu/matrixinput#671
izulin merged 18 commits into
developfrom
pu/matrixinput

Conversation

@izulin

@izulin izulin commented May 16, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Context

Adds parsing of arrays.

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.

@izulin
izulin requested a review from wojciechczerniak May 16, 2021 13:34
@lgtm-com

lgtm-com Bot commented May 16, 2021

Copy link
Copy Markdown
Contributor

This pull request introduces 1 alert when merging 027597f into b5b1717 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@codecov

codecov Bot commented May 16, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #671 (50b7aa2) into develop (6fa43f8) will decrease coverage by 0.48%.
The diff coverage is 69.03%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #671      +/-   ##
===========================================
- Coverage    92.85%   92.36%   -0.49%     
===========================================
  Files          166      166              
  Lines        38912    39089     +177     
  Branches      5387     5349      -38     
===========================================
- Hits         36131    36106      -25     
- Misses        2745     2947     +202     
  Partials        36       36
Impacted Files Coverage Δ
src/parser/ParserConfig.ts 0.00% <0.00%> (ø)
src/interpreter/Interpreter.ts 51.42% <8.82%> (-3.18%) ⬇️
src/dependencyTransformers/Transformer.ts 79.35% <16.66%> (-1.04%) ⬇️
src/parser/ParserWithCaching.ts 79.60% <25.00%> (-0.88%) ⬇️
src/NamedExpressions.ts 94.49% <33.33%> (-1.81%) ⬇️
src/MatrixSize.ts 51.05% <35.71%> (-2.76%) ⬇️
src/parser/Unparser.ts 65.27% <50.00%> (-7.58%) ⬇️
src/parser/Cache.ts 90.72% <66.66%> (-2.90%) ⬇️
src/parser/LexerConfig.ts 96.70% <79.31%> (-3.30%) ⬇️
src/Config.ts 98.80% <100.00%> (+0.05%) ⬆️
... and 31 more

@izulin izulin mentioned this pull request May 18, 2021
Closed
voodoo11
voodoo11 reviewed May 19, 2021
View reviewed changes
Comment thread src/interpreter/Interpreter.ts
rowRet[i].push(...arr.data[i])
}
} else {
return new CellError(ErrorType.REF, ErrorMessage.SizeMismatch)

@voodoo11 voodoo11 May 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would say it's ErrorType.VALUE

@izulin izulin May 19, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2021-05-19 at 10 44 31

@lgtm-com

lgtm-com Bot commented May 19, 2021

Copy link
Copy Markdown
Contributor

This pull request introduces 1 alert when merging b0dc57f into b5b1717 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com

lgtm-com Bot commented May 24, 2021

Copy link
Copy Markdown
Contributor

This pull request introduces 1 alert when merging be763b4 into e77bca5 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

wojciechczerniak
wojciechczerniak suggested changes May 27, 2021
View reviewed changes
Comment thread src/Config.ts
Comment on lines 635 to 644
configCheckIfParametersNotInConflict(
{value: this.decimalSeparator, name: 'decimalSeparator'},
{value: this.functionArgSeparator, name: 'functionArgSeparator'},
{value: this.thousandSeparator, name: 'thousandSeparator'}
{value: this.thousandSeparator, name: 'thousandSeparator'},
)

configCheckIfParametersNotInConflict(
{value: this.matrixRowSeparator, name: 'matrixRowSeparator'},
{value: this.matrixColumnSeparator, name: 'matrixColumnSeparator'},
)

@wojciechczerniak wojciechczerniak May 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can't those parameters be in conflict together? ie. I set the functionArgSeparatorand matrixColumnSeparator both as ; ?

@izulin izulin May 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

parser will handle this

@izulin izulin May 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

as in: parser already handles well the conflict those have over ',' symbol

@wojciechczerniak wojciechczerniak May 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Comment thread src/parser/FormulaParser.ts Outdated
@izulin @wojciechczerniak
Update src/parser/FormulaParser.ts
1f8e21f 
Co-authored-by: Wojciech Czerniak <wojciech.czerniak@gmail.com>
@lgtm-com

lgtm-com Bot commented May 27, 2021

Copy link
Copy Markdown
Contributor

This pull request introduces 1 alert when merging 1f8e21f into 831104d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com

lgtm-com Bot commented May 27, 2021

Copy link
Copy Markdown
Contributor

This pull request introduces 1 alert when merging cec1c20 into 831104d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com

lgtm-com Bot commented May 27, 2021

Copy link
Copy Markdown
Contributor

This pull request introduces 1 alert when merging 03b3bfc into 00ccf16 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

wojciechczerniak
wojciechczerniak approved these changes May 27, 2021
View reviewed changes

@wojciechczerniak wojciechczerniak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🥇

@lgtm-com

lgtm-com Bot commented May 27, 2021

Copy link
Copy Markdown
Contributor

This pull request introduces 1 alert when merging 1a5d0c8 into 9f96603 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com

lgtm-com Bot commented May 27, 2021

Copy link
Copy Markdown
Contributor

This pull request introduces 1 alert when merging 50b7aa2 into 6fa43f8 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@izulin
izulin merged commit 94c3d72 into develop May 27, 2021
@izulin
izulin deleted the pu/matrixinput branch May 27, 2021 18:15
@AMBudnik AMBudnik mentioned this pull request Jul 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@voodoo11 voodoo11 voodoo11 left review comments
@wojciechczerniak wojciechczerniak wojciechczerniak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@izulin @voodoo11 @wojciechczerniak