pboettch / json-schema-validator

JSON schema validator for JSON for Modern C++
Other
463 stars 133 forks source link

fix: validate multipleOf fails on float-point value #295

Closed dhmemi closed 5 months ago

dhmemi commented 7 months ago

Closes #293

Fix validating of float-point value.

dhmemi commented 7 months ago

The ci failed, but where I can get the details? The actions' outputs seem unhelpful.

pboettch commented 7 months ago

It is no longer a problem generated by your code. The problem is coming from the github-runner which are using cmake 3.27 and nlohmann::json which allows CMake 3.1. This is no longer compatibile.

I really don't know what to do now.

JacobCrabill commented 7 months ago

@pboettch I'd recommend trying to use pip3 to install a specific version of CMake in your CI script, rather than using the current template. Something like:

      - name: Get cmake == 3.24.1
        run: |
          apt install -y python3-pip
          pip3 install cmake==3.24.1
pboettch commented 7 months ago

@pboettch I'd recommend trying to use pip3 to install a specific version of CMake in your CI script, rather than using the current template. Something like:

      - name: Get cmake == 3.24.1
        run: |
          apt install -y python3-pip
          pip3 install cmake==3.24.1

All of the CI workflow has only been integrated but done by others. I'm grateful, but have to admin, that I don't really know where to put it. Could you help out with a PR?

Thanks.

pboettch commented 7 months ago

Well, maybe i found how to do it. See #296

JacobCrabill commented 7 months ago

All of the CI workflow has only been integrated but done by others. I'm grateful, but have to admin, that I don't really know where to put it. Could you help out with a PR?

I'll see what I can do to help out - I have a decent amount of experience with various CI systems

pboettch commented 7 months ago

I'm closing this one in favor of #297

pboettch commented 7 months ago

Could you please rebase your code on top of master and run tests locally. The multipleOf-tests fails for all versions for me locally.

pboettch commented 7 months ago

I rebased and pushed to your branch to your repo.

dhmemi commented 7 months ago

fixed.

As the multipleOf_.second is always float type, the method of calculating the error by rounding the quotient always works.

dhmemi commented 7 months ago

Seems still buggy, do not merge even if ci passed, I'll add more cases.

dhmemi commented 5 months ago

This branch is ready to be merged if there are no further suggested modifications.

I have adjusted the logic for determining whether a floating-point number is a multiple of another number based on the difference in multiples of the values specified in the value and multipleOf fields. While the criteria for such determination have become less strict, this should be more practical for most cases. It helps to avoid validation failures caused by slight floating-point drift resulting from simple numerical calculations.

pboettch commented 5 months ago

Thanks a lot for your work.