V0.5.4 - Githubissues

mountainMath / cancensus

R wrapper for calling CensusMapper APIs

https://mountainmath.github.io/cancensus/index.html

Other

82 stars 15 forks source link

V0.5.4 #188

Closed mountainMath closed 2 years ago

mountainMath commented 2 years ago

Release candidate addressing https://github.com/mountainMath/cancensus/issues/187 and https://github.com/mountainMath/cancensus/issues/184.

Enhaancement https://github.com/mountainMath/cancensus/issues/185 is probably more of a longer term project, but we could try and work at least the CSV xtabs into this one too.

dshkol commented 2 years ago

Looks pretty good to me now. A couple things:

There was a pretty big bug in the spatial format check that I'm surprised no one got caught on except for someone that emailed me directly. If someone didn't have 'sf' package and requested tabular data only -- spatial_format = NA -- we'd error them out. We didn't catch it because we obviously have that package installed in our systems, so I removed and tested some new versions of that check against a cleaner environment. Should be good now...
... but in doing so I realized that we either need to spend quite a bit of time to make everything that's been added in the past couple years backwards compatible to work with 'sp' format objects or move to drop support for that outright. With the entire 'sp' ecosystem now more or less moved on to 'sf' this would be my preferred option.
Due to the timeline you want to have for this release, I've just put a quick and dirty workaround that directly warns and prompts users to install 'sf' format if they request 'sp' format and don't have it installed. It won't break things for them any more so than any other transformation we've added recently.
For 0.5.5 we will either need to go through a bit more carefully and remove reference to sp or make it more immediately obvious that this is not supported anymore... or spend the time and make sure we are more careful that all the additional transformations added are checking and supporting for 'sp'.

mountainMath commented 2 years ago

I am comfortable with dropping support for 'sp' and only return in 'sf'. I have seen other packages do similar things. The 'sf' package is mature enough now that we can force that. And if people want it in 'sp' format that's an easy conversion at the end.

dshkol commented 2 years ago

I am comfortable with dropping support for 'sp' and only return in 'sf'. I have seen other packages do similar things. The 'sf' package is mature enough now that we can force that. And if people want it in 'sp' format that's an easy conversion at the end.

In this version or just fire this one off and focus on proper deprecation in the next one?

mountainMath commented 2 years ago

I say let's not worry about it in this round and deal with it next time.

dshkol commented 2 years ago

Cool. Wanna do a last quick check on your end? I’ll submit now for tests on win and rhub and then throw on cran when I’m home tonight

On Sun, Nov 6, 2022 at 12:04 PM Jens von Bergmann @.***> wrote:

I say let's not worry about it in this round and deal with it next time.

— Reply to this email directly, view it on GitHub https://github.com/mountainMath/cancensus/pull/188#issuecomment-1304882239, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMTTZ476PBPFGKKMHXT7JTWG76NBANCNFSM6AAAAAARLW3JIU . You are receiving this because your review was requested.Message ID: @.***>

mountainMath commented 2 years ago

Something went wrong in your fix spatial format check commit, it removed a bunch of functionality and also breaks my code.

dshkol commented 2 years ago

Something went wrong in your fix spatial format check commit, it removed a bunch of functionality and also breaks my code.

Ok, confusing, not sure why there's so many diffs. It should be just that section and the one that was previously there immediately above of ~10 lines.

mountainMath commented 2 years ago

Rolled back the changes and added the part about the spatial format check back in. Can you check to make sure it works as you intended?

dshkol commented 2 years ago

Rolled back the changes and added the part about the spatial format check back in. Can you check to make sure it works as you intended?

yeah, this appears to work as intended and I'm confused where the additional diffs came from

dshkol commented 2 years ago

Can you try testing with sf installed and without?

dshkol commented 2 years ago

Can you try testing with sf installed and without?

Let me know if this works for you and I'll submit. Seems to pass my tests.

mountainMath commented 2 years ago

Works. I like the menu.

dshkol commented 2 years ago

Submitted.

On Sun, Nov 6, 2022 at 7:37 PM Jens von Bergmann @.***> wrote:

Works. I like the menu.

— Reply to this email directly, view it on GitHub https://github.com/mountainMath/cancensus/pull/188#issuecomment-1305038855, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMTTZ6XJCHNIXMCRM5TFATWHBTP3ANCNFSM6AAAAAARLW3JIU . You are receiving this because your review was requested.Message ID: @.***>

dshkol commented 2 years ago

Submitted.

on CRAN now. We can merge and tag.