gpsbabel icon indicating copy to clipboard operation
gpsbabel copied to clipboard

unicsv fixes

Open robertlipe opened this issue 2 years ago • 5 comments

  • Changes to unicsv to improve robustness, allow read of Sidewalk 360 Eval files.
  • Remove debug chatter.
  • Allow DateTime to work in a unicsv column. All the hard work was there. The name "datetime" just wasn't hooked up to set the bits for the scanner.

robertlipe avatar Apr 11 '23 07:04 robertlipe

While your in here can you fix the extra QString/char* roundtrip in these two places (human_to_dec takes a QString): https://github.com/GPSBabel/gpsbabel/blob/cad16e14c34f3ee16fde021aa4df82b2461f5bff/unicsv.cc#L544-L549

I dislike using the null island as flag, it exists. Ok, I grant you it rarely occurs. What happens if either lat or lon is 999 but not both? Shouldn't we throw out points that are missing either a latitude or a longitude? (undo https://github.com/GPSBabel/gpsbabel/pull/1064/commits/6aa2d365162087e9a337ad2c9c3b44aa94c38070) Should we modernize the parameter passing in human_to_dec? Can we return a std::pair<std::optional, std::optional> instead of passing pointers for outlat, outlon?

tsteven4 avatar Apr 11 '23 13:04 tsteven4

Good points. I hated the calling convention while I was looking at it, but that function is just terrifying enough that I didn't want to get too radical with it. I thought there were more callers of this than there really are, so I really should just go make a better signature and fix them all. This function has the really weird convention of returning either or both, but we can work with that.

Importantly (I couldn't find it last night...) the code coverage of that function is better than I thought: https://app.codacy.com/gh/GPSBabel/gpsbabel/file/86982252082/coverage?bid=26148649&fileBranchId=26148649

The reason for https://github.com/GPSBabel/gpsbabel/commit/6aa2d365162087e9a337ad2c9c3b44aa94c38070 is exactly to handle points on the prime meridian OR the equator. We have some of these in our testsuite where we hand-created some polygons with coordinates of (3, 2) and (1, 0) degrees or something. With an actual return value on h_to_d we can at least issue a warning at the point of failure within the line (though we don't have line number or column number) but it's only really at the end of the line that we can tell if both set_lat and set_long have been called and we have a fully formed point. Maybe we can stuff sentinels in the newly created waypt_tmp when we created it and look to confirm they've been both changed before we proceed.

I'll think more on this point.

GPSBabelDeveloper avatar Apr 12 '23 04:04 GPSBabelDeveloper

Coverage summary from Codacy

Merging #1064 (7050664d1a01be77cb3e526b7f0462895c21851a) into master (cad16e14c34f3ee16fde021aa4df82b2461f5bff) - See PR on Codacy

Coverage variation Diff coverage
-0.03% 67.57%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (cad16e14c34f3ee16fde021aa4df82b2461f5bff) 24420 16522 67.66%
Head commit (7050664d1a01be77cb3e526b7f0462895c21851a) 24451 (+31) 16535 (+13) 67.63% (-0.03%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#1064) 74 50 67.57%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

codacy-production[bot] avatar May 14 '23 08:05 codacy-production[bot]

Oh. Now we have an actual problem.

My code that oh-so-cleverly detects malformed unicsv that results in 0 lat long works perfectly ... and tosses hundreds of 0,0 data that appears in our test suite, notably as test cases for mkshort.

Maybe we skootch those off 0,0 a bit to 0.1, 0.1 or something? I'd have to see if any of the other problems are "real".

Rats!

robertlipe avatar Sep 14 '24 13:09 robertlipe

Can we add a Sidewalk 360 Eval file to test?

tsteven4 avatar Sep 14 '24 13:09 tsteven4