-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Alter column metadata fields #3717
Conversation
@seancolsen Can you please add the appropriate docstrings for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Anish9901. That was exactly what I asked for.
It turns out I've cycled through some different approaches to the options for num_grouping
. And — embarrassingly — that vacillation is reflected in some commits I pushed here.
- In
master
, the options fornum_grouping
are'true' | 'false'
(stringified booleans). Yes, weird. Long story. - I wanted to fix this in my current work, and I first though of
'force-yes' | 'force-no'
. That's when I drafted this request for you (queued up with some other ones). It turns out the specific approach went stale. Sorry! - Then I changed my mind to a boolean and wrote some code comments to that effect.
- Then I changed my mind again and settled on
'always' | 'auto' | 'never'
. But I was moving too fast and didn't update the code comment. Ooops.
In trying to clean this up, I accidentally looked at my outdated code comment. And I ended up pushing 7116e7f to this PR to implement approach 3.
Whoops! Now I've pushed for 4.
. Oy vey! Been a long day.
Finally I pushed f96a9d2 to squash all these into one migration.
Can you look over this to make sure it's sane?
I'd rather not bother with documenting these parameters in metadata.py
. They're not straightforward to explain. Documenting them in these docstrings is really quite tedious and has left me with the opinion that I'm not even sure the whole RPC documentation system is really worth it right now. I have documented the parameters quite thoroughly in the front end here. You can see how much I wrote there. It's crazy to me that we have over 100 lines of python code in metadata.py
just to produce API docs for a very small portion of the API.
4f5fd8e
to
f4aec66
Compare
Your changes look good to me @seancolsen. Regarding migrations, you could've deleted the migrations file in this PR, made changes to the models and then run Regarding choices for fields, I discovered that these choices aren't enforced on the underlying db and serve as just documentation for now, if we do want these choices to be enforced we'd have to call |
Funny. I also noticed that while testing this PR yesterday. I actually added it to the technical beta planning meeting: Validation of metadata values I'm good with merging this PR now. I'll let you merge when you're ready. |
Fixes #3714
Checklist
Update index.md
).develop
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin