omissis / go-jsonschema

A tool to generate Go data types from JSON Schema definitions.
MIT License
582 stars 93 forks source link

enhance splitIdentifierByCaseAndSeparators to support non-case-sensitive languages #170

Closed zrma closed 9 months ago

zrma commented 11 months ago

This PR includes modifications to the splitIdentifierByCaseAndSeparators function, aiming to extend its compatibility to non-case-sensitive languages such as Korean, Chinese, and Japanese, among others.

Key Points:

However, I faced difficulties in augmenting the test suite to include these new language scenarios. My limited experience with the current test structure hindered my ability to confidently implement new test cases for these languages.

In an attempt to modify the tests, I utilized the go-jsonschema binary, executing the following command:

./go-jsonschema --capitalization HtMl,ID,URL -p test tests/data/misc/capitalization/capitalization.json -o tests/data/misc/capitalization/capitalization.go

This process resulted in struct names being generated as CapitalizationJson instead of Capitalization. Consequently, I manually adjusted the names.

I am uncertain about the correctness of these steps and would greatly appreciate any advice or guidance. Your feedback or suggestions for improving this feature expansion and the addition of test cases would be invaluable.

Thank you.

codecov[bot] commented 11 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (33ec559) 75.89% compared to head (2aa55c8) 76.51%. Report is 15 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #170 +/- ## ========================================== + Coverage 75.89% 76.51% +0.61% ========================================== Files 24 24 Lines 1871 1882 +11 ========================================== + Hits 1420 1440 +20 + Misses 361 353 -8 + Partials 90 89 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

omissis commented 10 months ago

hi @zrma , thanks a lot for the contribution! I will try to take a look at it over the xmas/nye holidays and hopefully merge it! 🙏

zrma commented 9 months ago

Hi, thanks again for the contribution and for the patience in waiting for the review! 🙏 The code looks good to me: if you could just push those little naming fixes I added to the PR I can then merge it! 🥂 Unfortunately I have no knowledge of non-case-sensitive languages, but if any user who does will find any bug we could fix it when the need arises. ✌️

I've made the suggested naming changes as per your review. Thank you for the valuable feedback and guidance! 😻 The updated commit has been pushed to the PR.