espressif / idf-extra-components

Additional components for ESP-IDF, maintained by Espressif
147 stars 89 forks source link

json_parser: add idf_version rules for jsmn #140

Closed tswen closed 1 year ago

tswen commented 1 year ago

Checklist

Change description

Fix a compilation error of json_parser component on idf v4.x.

Because the jsmn component was part of idf v4.4 and earlier.

The compile errors were:

  ERROR: Cannot process component requirements.  Multiple candidates to
  satisfy project requirements:

    requirement: "jsmn" candidates: "espressif__jsmn, jsmn"
CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

igrr commented 1 year ago

@shahpiyushv PTAL

shahpiyushv commented 1 year ago

@tswen , can you add some explanation as to why this is being done. I myself know that jsmn was part of idf earlier, but is not there anymore and so pulling this component must be causing component duplication. But, it would be better to have this info in commit message too.

Meanwhile, have you tested if this works well with the jsmn from idf v4.x?

igrr commented 1 year ago

Meanwhile, have you tested if this works well with the jsmn from idf v4.x?

@shahpiyushv could you suggest a simple smoke test case for json_parser component? Then we could add it to the repo and run the test automatically, ensuring that the component works with all the supported versions of IDF.

igrr commented 1 year ago

@tswen please click through the CLA as well. (This is not strictly necessary since you are an employee, but it's just easier to get all the status checks green this way.)

shahpiyushv commented 1 year ago

Meanwhile, have you tested if this works well with the jsmn from idf v4.x?

@shahpiyushv could you suggest a simple smoke test case for json_parser component? Then we could add it to the repo and run the test automatically, ensuring that the component works with all the supported versions of IDF.

There is actually a generic test code in the original repo if someone wants to adapt it for esp-idf: https://github.com/espressif/json_parser/blob/master/tests/main.c. If there are some pointers to help add the tests here, I can do it myself too.

igrr commented 1 year ago

@shahpiyushv I guess it could look like this — very similar to the version in your repo, just wrapped into a TEST_CASE macro and with a bunch of TEST_ASSERTs instead of printfs, so that the results can be verified automatically.

tswen commented 1 year ago

@shahpiyushv I tested the change on idf 4.x and it works well.

igrr commented 1 year ago

@tswen This change looks good to me! As @shahpiyushv said, please update the Pull Request description to explain the reason behind this change. Then it should be ready it merge.

tswen commented 1 year ago

@igrr @shahpiyushv Thanks to your review suggestions, I've updated the change description.

igrr commented 1 year ago

@tswen Have merged, thank you for the contribution!