Skip to content
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

validation for abcd_scenario_* objects #28

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Conversation

cjyetman
Copy link
Member

@cjyetman cjyetman commented Jan 3, 2023

The intent is to add validation functions for scenario data objects, following the pattern of the many other validation functions that are the bread and butter of this package. It also includes all the infrastructure to make that happen well: documentation, faker functions, and tests.

This PR has been paused until we know that the scenario data object form has been stabilized.

@cjyetman cjyetman marked this pull request as draft January 3, 2023 10:58
@jdhoffa
Copy link
Member

jdhoffa commented Jan 4, 2024

This is confusing. It says it was opened yesterday, but then the commits are from 2 years ago?

Is the PR stale or active?

@cjyetman
Copy link
Member Author

cjyetman commented Jan 4, 2024

This is from many months ago (~10?), but not 2 years. It is "stale" in the sense of having been open for a long time, but because little to nothing has changed in the main branch since then either, that's not necessarily a bad thing.

@jdhoffa
Copy link
Member

jdhoffa commented Jan 5, 2024

My main worry is not that it should fall out of sync with main, but rather that the PR has no description, so I feel the context of what is meant to happen may get lost.

I would imagine one of the following approaches may be desirable:
Merge it as-is (obviously validation functions can be worked on forever, so is this at an ok level)
Should we document the PR (so that when we come back to it in another year, we remember what is meant to be going on)
Close it

BUT, that said, you are the maintainer of this repo so if you are keen to just keep the draft open as-is then I have no problem with that :-)

@cjyetman
Copy link
Member Author

cjyetman commented Jan 5, 2024

The intent is to add validation functions for scenario data objects, following the pattern of the many other validation functions that are the bread and butter of this package. It also includes all the infrastructure to make that happen well: documentation, faker functions, and tests.

It is mostly complete, but I paused on it because I wasn't sure if the scenario data object form is/was stable... and if it changes/has changed from what it was when I wrote this, then these functions become immediately irrelevant.

@cjyetman
Copy link
Member Author

cjyetman commented Jan 5, 2024

If I imagine a few scenarios...

  1. We merge this as is. The scenario object form was not stable and the functions are immediately irrelevant. Once it is stabilized and we figure out what to do we come back and make a new PR to fix the code that has already been merged.
  2. We close this PR and delete the branch. Once the scenario object form is stabilized and we know what to do, we come back here and start from scratch.
  3. We leave this PR in draft. Once the scenario object form is stabilized and we know what to do, we come back here and make minor modification to this PR that is already almost complete.

Of those three scenarios, I prefer the third.

@jdhoffa
Copy link
Member

jdhoffa commented Jan 15, 2024

Sounds good. Third it is :-)

@jdhoffa
Copy link
Member

jdhoffa commented Apr 24, 2024

Revisiting this thread, it seems that the abcd_scenario_* object hasn't changed too much in 3+ years, so I guess it's more or less stable?? ahaha

@cjyetman do you think it would make sense to adjust/ merge this?

I know there may be a time where we want to split that object up, but I don't anticipate it's going to happen in the short- or even medium- term

@cjyetman
Copy link
Member Author

I think it still needs some love, but I think we can put it back on the agenda.

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