Skip to content
This repository has been archived by the owner on Jul 11, 2022. It is now read-only.

Autocomplete number when needed #141

Merged
merged 4 commits into from
May 3, 2021
Merged

Conversation

Nexucis
Copy link
Member

@Nexucis Nexucis commented May 3, 2021

fix #136

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
@Nexucis Nexucis requested a review from juliusv May 3, 2021 14:04
Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
@@ -445,6 +445,11 @@ export const aggregateOpModifierTerms = [
},
];

export const numberTerms = [
{ label: 'nan', info: 'not a number', type: 'constant' },
Copy link
Member

Choose a reason for hiding this comment

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

I'd go with:

Suggested change
{ label: 'nan', info: 'not a number', type: 'constant' },
{ label: 'NaN', info: 'Floating-point NaN value', type: 'constant' },

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -445,6 +445,11 @@ export const aggregateOpModifierTerms = [
},
];

export const numberTerms = [
{ label: 'nan', info: 'not a number', type: 'constant' },
{ label: 'inf', info: 'the infinite and beyond of the number', type: 'constant' },
Copy link
Member

Choose a reason for hiding this comment

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

I'd go with:

Suggested change
{ label: 'inf', info: 'the infinite and beyond of the number', type: 'constant' },
{ label: 'Inf', info: 'Floating-point infinity', type: 'constant' },

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -330,12 +336,22 @@ export function analyzeCompletion(state: EditorState, node: SyntaxNode): Context
{ kind: ContextKind.Function },
{ kind: ContextKind.Aggregation }
);
if (parent.type.id !== FunctionCallArgs && parent.type.id !== MatrixSelector) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, when I type n in a function call parameter, it now doesn't autocomplete nan, but should. Is it because of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yeah that's possible. It was more because we don't want to autocomplete the number in the the sum for example.
But yeah I forgot that sometimes it's possible ....

Maybe we should just autocomplete it everytime even if it's sometimes false depending of the aggregation / function

Copy link
Member Author

Choose a reason for hiding this comment

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

even if I don't really like it :/.

But well in the other side, putting NaN / Inf is a bit a corner case in the function call parameters right ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's legal in the type system and will cause a valid query, but I'm not aware of any valid use case with the current set of functions taking scalars :) So probably it's fine to just not support it for now, yeah.

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

Co-authored-by: Julius Volz <julius.volz@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #141 (e75db8b) into master (8e55568) will increase coverage by 0.15%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #141      +/-   ##
==========================================
+ Coverage   72.56%   72.72%   +0.15%     
==========================================
  Files          15       15              
  Lines         740      748       +8     
  Branches      166      167       +1     
==========================================
+ Hits          537      544       +7     
- Misses        124      125       +1     
  Partials       79       79              
Impacted Files Coverage Δ
src/lang-promql/complete/hybrid.ts 75.72% <87.50%> (+0.29%) ⬆️
src/lang-promql/complete/promql.terms.ts 100.00% <100.00%> (ø)

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 8e55568...e75db8b. Read the comment docs.

@juliusv
Copy link
Member

juliusv commented May 3, 2021

👍

@juliusv juliusv merged commit 197c219 into master May 3, 2021
@juliusv juliusv deleted the feature/autocomplete-number branch May 3, 2021 14:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support autocompleting NaN and Inf
3 participants