-
Notifications
You must be signed in to change notification settings - Fork 65
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
Added a linter for numeric types #800
base: main
Are you sure you want to change the base?
Conversation
Changes AnalysisCommit SHA: 2052087 API ChangesSummary
ReportThe full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/12913997773/artifacts/2469723529 API Coverage
|
It's certainly not expected! If I were brave and wanted to get to the bottom of this, I would edit the code to return the full error, then run it locally and in CI to find what the actual root cause is with no other changes. I also ran into it in #793 (not yet merged) and updated the test for now. I am guessing a library version must be different or the list of errors is sorted differently depending on some transient environment / operating system / node version. The JSON schema validator fails correctly, but the error messages returned vary in order, and I believe we truncate them, so you get a different output in CI vs. locally. |
Thank you so much! I actually don’t have any idea why it’s happening. It gives me ideas on how to work with it. |
Spec Test Coverage Analysis
|
I am happy to look into it next week if you can't figure it out. |
@Tokesh I'd recommend syncing the latest main branch into your fork and rebasing your branch onto it. CI builds usually try to run as if your branch has been merged and so can sometimes have different results compared to your local clone. |
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
4dc2b72
to
4930421
Compare
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Finally, thank you so much for help and suggestions! @dblock @Xtansia |
pages_processed: | ||
type: number | ||
format: double | ||
documents_processed: | ||
type: number | ||
format: double | ||
documents_indexed: | ||
type: number | ||
format: double | ||
index_time_in_millis: | ||
type: number | ||
format: double | ||
search_time_in_millis: | ||
type: number | ||
format: double |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blindly adding format: double
to any type: number
without a format is not ideal as for example none of these should actually be doubles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I initially wanted to fix each one individually, but there were around 500 errors in the Validate CI. One idea was to create an issue and fix them gradually over time, or, as a temporary solution, set everything to double
or int32
by default for now.
I'm open to any suggestions from you. In any case, I made this change just to ensure that the CI passes successfully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neither Ruby nor JS generator takes into account format
right now. They are only translated to the default Integer and Number. In Ruby's case, the types are only generated for documentation purpose. In JS, format does not affect type checking for TS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One idea was to create an issue and fix them gradually over time, or, as a temporary solution, set everything to
double
orint32
by default for now.
Setting everything to double would make it hard to go back and figure out which one has been verified to be an actual double, and which should be corrected to float. If you just leave them blank, it's easier to tell which ones need to be looked into.
The task of determining the format of every numerical schema is going to take a lot of time and effort. Is there a need for that right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not be the ask from the original issue. Waiting on verification.
if (schema.format == null || !schema.format) { | ||
errors.push(ctx.error(`Schema of type 'number' must specify a valid format. Allowed formats: ${SCHEMA_NUMBER_FORMATS.join(', ')}`)); | ||
return; | ||
} | ||
|
||
if (SCHEMA_INTEGER_FORMATS.includes(schema.format)) { | ||
errors.push(ctx.error(`schema of type 'number' with format '${schema.format}' should instead be of type 'integer'`)) | ||
} else { | ||
errors.push(ctx.error(`schema of type 'number' with format '${schema.format}' is invalid, expected one of: ${SCHEMA_NUMBER_FORMATS.join(', ')}`)) | ||
if (!SCHEMA_NUMBER_FORMATS.includes(schema.format)) { | ||
errors.push(ctx.error(`Schema of type 'number' with format '${schema.format}' is invalid. Expected one of: ${SCHEMA_NUMBER_FORMATS.join(', ')}`)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're only allowing non-null valid format then the first if
on lines 74-77 are redundant because SCHEMA_NUMBER_FORMATS
doesn't include null
or false
, and the error in the first if
will be triggered in the second if
anyway.
Description
Checking numeric types
Issues Resolved
[#617]
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.