refactor(city): inject CSV data into get_city and iter_cities (#106)#112
refactor(city): inject CSV data into get_city and iter_cities (#106)#112costajohnt wants to merge 2 commits into
Conversation
…e#106) Both functions now take a data buffer and length instead of reading from the cities.h globals. main.c passes cities, cities_len so end-user behavior is unchanged. The payoff is in tests. A new test_get_city_duplicate_names feeds a two-row fixture with the same name in both possible orderings and verifies the higher-population row always wins, which is the case called out in the issue after PR da-luce#79. Closes da-luce#106
Reflow iter_cities signature and the test CSV string-literal initializers so clang-format --check passes under the pinned CI version.
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
|
Circling back on this refactor, no worries if it isn't a priority right now. Happy to rebase it against main or close it depending on what makes sense for the project. |
|
Hi @costajohnt, I promise I have't abandoned the project 😅 The PR is exactly what the issue is asking for, so no need to close. I recently started a new internship and have been really pre-occupied with work. Approving PRs doesn't take much time, but dealing with github CI can be really annoying (runner versions deprecated, other random infra breaks, etc.), so I haven't had the bandwidth to deal with it lately. I'll get this merged as soon as I find some more free time! 😁 |
|
No worries at all, and congrats on the new internship! That first stretch always eats all your time. Totally get the CI pain too. If it'd help take some of that off your plate, I'm happy to send a small separate PR that bumps the deprecated runner versions / GitHub Actions so the pipeline goes green again, whenever you want to tackle it. No rush on any of this, just glad the refactor still fits. Thanks for the update! |
2 participants
Summary
Un-hardcodes
get_cityanditer_citiesper issue #106. Both functions now take a CSV byte buffer and its length as explicit parameters instead of reading thecities.hglobals directly.main.cpassescities, cities_lento both call sites, so end-user behavior is unchanged.test/city_test.cupdates to pass the same arguments for all existing tests, confirming the default path still works.Why
The issue calls out that duplicate-name handling is hard to test when the function only reads from a compiled-in global. The only way to stress it is via the full
cities.csvand we can't easily guarantee both orderings of a given name are present.The new test
test_get_city_duplicate_namesfeeds two tiny two-row CSV fixtures with aLondonin each ordering:and asserts the ~8.9M row wins either way. This would have caught the binary-search regression fixed in PR #79 directly.
Also added
test_iter_cities_null_data_should_not_crashsince the new parameter adds a new null-handling path.Test plan
meson test -C build city_test— 6/6 pass (was 4/4 before, +2 new)meson test -C build— 8/8 suites pass./build/astroterm --help— binary still runscities.csvorcities.hgenerationCloses #106