Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Managing fallout from moving to the new sasdata package #2172

Closed
butlerpd opened this issue Aug 31, 2022 · 3 comments
Closed

Managing fallout from moving to the new sasdata package #2172

butlerpd opened this issue Aug 31, 2022 · 3 comments
Labels
Blocker Prevents a different issue from being resolved Enhancement Feature requests and/or general improvements

Comments

@butlerpd
Copy link
Member

Is your feature request related to a problem? Please describe.
With the merge today of the PR #2141 (and associated sasmodels PR) all scripts using the SasView dataloader are now be broken. There are probably quite a lot of these.

Describe the solution you'd like
This should be a discussion item. Of course we could do nothing (though I'd argue that is bad form). The options I see would be

  • document the breaking change prominently in the release notes and otherwise do nothing
  • IF we want to move to a more strict semantic versioning then does this count as a breaking change? I would argue yes in which case we would need to skip 5.1 and go to 6.0. Do we want to do that?
  • Alternatively, continuing on with the stricter semantic versioning, we could add a "shim" in SasView that captures "calls" to the old dataloader and redirects. In this case we may want to specify a sunset (deprecation) with a goal of removing the "shim" come version 6.0?

Having written it out, I now think the last option is the correct option

Describe alternatives you've considered
See above -- discussion item

Additional context
NONE

@butlerpd butlerpd added Blocker Prevents a different issue from being resolved Feature Request labels Aug 31, 2022
@lucas-wilkins
Copy link
Contributor

lucas-wilkins commented Aug 31, 2022

Yeah, its a pretty big change, I vote to bump it to 6.0, and bump sasmodels to 2.0 too.

Alternatively, continuing on with the stricter semantic versioning, we could add a "shim" in SasView that captures "calls" to the old dataloader and redirects.

How would you see this working? An __init__.py with references to sasdata as an external package? or are you suggesting we still include sasdata in sasview?

@lucas-wilkins lucas-wilkins added Enhancement Feature requests and/or general improvements and removed Feature Request labels Aug 31, 2022
@butlerpd
Copy link
Member Author

Good question :-) Not exactly sure but it seems like it would be relatively straightforward? 😄

Basically yes I guess on keeping dataloader base classes but make them import the sasdata package and then use that instead of the code currently being used? Spoken as someone who has NOT done this kind of thing before clearly...

That said we did some kind of ugly shimming I think for sasmodels which is why we still have a sasview sasmodels that does some kind of translation if I remember (and is on the list to be replaced/removed at some point ... indicating perhaps it is not so easy to get rid of :-)

@lucas-wilkins
Copy link
Contributor

Converting to a discussion.

@SasView SasView locked and limited conversation to collaborators Sep 12, 2022
@lucas-wilkins lucas-wilkins converted this issue into discussion #2187 Sep 12, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
Blocker Prevents a different issue from being resolved Enhancement Feature requests and/or general improvements
Projects
None yet
Development

No branches or pull requests

2 participants