Added a linter for numeric types #800

Tokesh · 2025-01-17T14:24:38Z

Description

Checking numeric types

Issues Resolved

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.

github-actions · 2025-01-17T14:26:36Z

Changes Analysis

Commit SHA: 5541725
Comparing To SHA: 5644cab

API Changes

Summary

└─┬Paths
  ├─┬/{index}/_bulk
  │ ├─┬PUT
  │ │ └─┬Requestbody
  │ │   └─┬application/x-ndjson
  │ │     └─┬Schema
  │ │       └─┬Schema
  │ │         ├──[➕] anyOf (45471:7)
  │ │         ├─┬ANYOF
  │ │         │ └──[🔀] type (27637:25)❌ 
  │ │         ├─┬ANYOF
  │ │         │ └──[🔀] $ref (27636:19)❌ 
  │ │         └─┬ANYOF
  │ │           └──[🔀] $ref (45413:13)❌ 
  │ └─┬POST
  │   └─┬Requestbody
  │     └─┬application/x-ndjson
  │       └─┬Schema
  │         └─┬Schema
  │           ├──[➕] anyOf (45471:7)
  │           ├─┬ANYOF
  │           │ └──[🔀] type (27637:25)❌ 
  │           ├─┬ANYOF
  │           │ └──[🔀] $ref (27636:19)❌ 
  │           └─┬ANYOF
  │             └──[🔀] $ref (45413:13)❌ 
  └─┬/_bulk
    ├─┬PUT
    │ └─┬Requestbody
    │   └─┬application/x-ndjson
    │     └─┬Schema
    │       └─┬Schema
    │         ├──[➕] anyOf (45471:7)
    │         ├─┬ANYOF
    │         │ └──[🔀] type (27637:25)❌ 
    │         ├─┬ANYOF
    │         │ └──[🔀] $ref (27636:19)❌ 
    │         └─┬ANYOF
    │           └──[🔀] $ref (45413:13)❌ 
    └─┬POST
      └─┬Requestbody
        └─┬application/x-ndjson
          └─┬Schema
            └─┬Schema
              ├──[➕] anyOf (45471:7)
              ├─┬ANYOF
              │ └──[🔀] type (27637:25)❌ 
              ├─┬ANYOF
              │ └──[🔀] $ref (27636:19)❌ 
              └─┬ANYOF
                └──[🔀] $ref (45413:13)❌

Document Element	Total Changes	Breaking Changes
paths	16	12

❌ BREAKING Changes: 12 out of 16
Modifications: 12
Additions: 4
Breaking Modifications: 12

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/13171020237/artifacts/2545036286

API Coverage

	Before	After	Δ
Covered (%)	663 (64.94 %)	663 (64.94 %)	0 (0 %)
Uncovered (%)	358 (35.06 %)	358 (35.06 %)	0 (0 %)
Unknown	45	45	0

Tokesh · 2025-01-17T18:03:26Z

I can't understand why the tests run without errors on two local computers, but fail on CI.

dblock · 2025-01-17T18:15:02Z

I can't understand why the tests run without errors on two local computers, but fail on CI.

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.

Tokesh · 2025-01-17T18:18:39Z

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.

github-actions · 2025-01-17T19:15:52Z

Spec Test Coverage Analysis

Total	Tested
595	593 (99.66 %)

dblock · 2025-01-18T13:44:22Z

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.

I am happy to look into it next week if you can't figure it out.

Xtansia · 2025-01-20T22:00:40Z

@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.

Tokesh · 2025-01-22T17:53:29Z

Finally, thank you so much for help and suggestions! @dblock @Xtansia
34 commits and a lot of debugging...
I'll never forget that bug that appeared due to an incorrect rebase, and I was trying to track it down everywhere
I think i need to write motivation post here for newbies:
"I fear not the man who has practiced 10,000 kicks once, but I fear the man who has practiced one kick 10,000 times" (c) Bruce Lee

Xtansia · 2025-01-22T21:47:24Z

spec/schemas/transforms._common.yaml

        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


Blindly adding format: double to any type: number without a format is not ideal as for example none of these should actually be doubles.

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.

@Xtansia @nhtruong what's the default handling of format in generators when we don't have it? We should safely default to that here, no?

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.

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.

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?

@dblock @nhtruong @Xtansia
I think I now understand the entire context, thanks to everyone for participating in my PR discussion. I suggest that I create an Issue where, sooner or later, we will try to specify the exact format for each number and integer. After we ensure that we've done this everywhere, we can merge this PR. (I will leave just the linter check here.) I think we, as maintainers, can at least start asking people to specify more accurate formats in future PRs so that the number of such inaccurate parameters doesn’t increase. At the very least, I’ll try to gradually fill in the formats of the specs during my free time.
WDYT?

I think this is a good plan. Thanks for working on this.

It's might be a tall order but we can add a new a linter that flags missing format for new number/integer schemas.

This is that PR, but it forces that requirement on all fields.

@Tokesh maybe this PR can emit a warning instead of an error?

I mean only for new schemas, which makes it hard because the linter will have to be able to detect which ones are new from the git diff.

nhtruong · 2025-01-31T16:27:12Z

tools/src/linter/SchemaVisitingValidator.ts

+      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(', ')}`));


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.

Signed-off-by: Tokesh <tokesh789@gmail.com>

Tokesh marked this pull request as ready for review January 17, 2025 18:01

Tokesh requested review from dblock, harshavamsi, sachetalva, nhtruong, Xtansia, VachaShah and aabeshov as code owners January 17, 2025 18:01

Tokesh marked this pull request as draft January 18, 2025 11:44

Tokesh mentioned this pull request Jan 20, 2025

[BUG] Strange behavior of unit tests in StoryValidator #802

Closed

Tokesh force-pushed the linter-numeric branch from 4dc2b72 to 4930421 Compare January 21, 2025 07:09

Tokesh marked this pull request as ready for review January 22, 2025 17:53

Xtansia requested changes Jan 22, 2025

View reviewed changes

nhtruong reviewed Jan 31, 2025

View reviewed changes

nhtruong mentioned this pull request Feb 3, 2025

[BUG] inaccurate numeric type #596

Open

Tokesh marked this pull request as draft February 6, 2025 02:37

Tokesh closed this Feb 6, 2025

Tokesh force-pushed the linter-numeric branch from 2052087 to 5644cab Compare February 6, 2025 03:00

feat: testing add vale rule

1285ddb

Signed-off-by: Tokesh <tokesh789@gmail.com>

Tokesh reopened this Feb 6, 2025

added: vale yml and changelog

5541725

Signed-off-by: Tokesh <tokesh789@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a linter for numeric types #800

Added a linter for numeric types #800

Tokesh commented Jan 17, 2025

github-actions bot commented Jan 17, 2025 •

edited

Loading

Tokesh commented Jan 17, 2025

dblock commented Jan 17, 2025

Tokesh commented Jan 17, 2025

github-actions bot commented Jan 17, 2025 •

edited

Loading

dblock commented Jan 18, 2025

Xtansia commented Jan 20, 2025

Tokesh commented Jan 22, 2025 •

edited

Loading

Xtansia Jan 22, 2025

Tokesh Jan 23, 2025

dblock Jan 25, 2025

nhtruong Jan 31, 2025 •

edited

Loading

nhtruong Jan 31, 2025

Tokesh Feb 3, 2025

dblock Feb 3, 2025

nhtruong Feb 3, 2025

dblock Feb 3, 2025

nhtruong Feb 5, 2025

nhtruong Jan 31, 2025

Added a linter for numeric types #800

Are you sure you want to change the base?

Added a linter for numeric types #800

Conversation

Tokesh commented Jan 17, 2025

Description

Issues Resolved

github-actions bot commented Jan 17, 2025 • edited Loading

Changes Analysis

API Changes

Summary

Report

API Coverage

Tokesh commented Jan 17, 2025

dblock commented Jan 17, 2025

Tokesh commented Jan 17, 2025

github-actions bot commented Jan 17, 2025 • edited Loading

Spec Test Coverage Analysis

dblock commented Jan 18, 2025

Xtansia commented Jan 20, 2025

Tokesh commented Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhtruong Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2025 •

edited

Loading

github-actions bot commented Jan 17, 2025 •

edited

Loading

Tokesh commented Jan 22, 2025 •

edited

Loading

nhtruong Jan 31, 2025 •

edited

Loading