Skip to content

V0.5.4#188

Merged
mountainMath merged 9 commits into
mainfrom
v0.5.4
Nov 7, 2022
Merged

V0.5.4#188
mountainMath merged 9 commits into
mainfrom
v0.5.4

Conversation

@mountainMath
Copy link
Copy Markdown
Owner

Release candidate addressing #187 and #184.

Enhaancement #185 is probably more of a longer term project, but we could try and work at least the CSV xtabs into this one too.

@mountainMath mountainMath requested a review from dshkol October 30, 2022 16:33
@dshkol
Copy link
Copy Markdown
Collaborator

dshkol commented Nov 6, 2022

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'.

Comment thread R/census_regions.R
regions
}

#' Convenience function for creating unique names from region list
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a useful helper function for the main power user of this package :)

@mountainMath
Copy link
Copy Markdown
Owner Author

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
Copy link
Copy Markdown
Collaborator

dshkol commented Nov 6, 2022

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
Copy link
Copy Markdown
Owner Author

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

@dshkol
Copy link
Copy Markdown
Collaborator

dshkol commented Nov 6, 2022 via email

@mountainMath
Copy link
Copy Markdown
Owner Author

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

@dshkol
Copy link
Copy Markdown
Collaborator

dshkol commented Nov 6, 2022

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
Copy link
Copy Markdown
Owner Author

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
Copy link
Copy Markdown
Collaborator

dshkol commented Nov 6, 2022

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
Copy link
Copy Markdown
Collaborator

dshkol commented Nov 6, 2022

Can you try testing with sf installed and without?

@dshkol
Copy link
Copy Markdown
Collaborator

dshkol commented Nov 7, 2022

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
Copy link
Copy Markdown
Owner Author

Works. I like the menu.

@dshkol
Copy link
Copy Markdown
Collaborator

dshkol commented Nov 7, 2022 via email

@dshkol
Copy link
Copy Markdown
Collaborator

dshkol commented Nov 7, 2022

Submitted.

on CRAN now. We can merge and tag.

@mountainMath mountainMath merged commit 4da898e into main Nov 7, 2022
@mountainMath mountainMath deleted the v0.5.4 branch November 7, 2022 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants