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

refactor _TypeDict #414

Closed
wants to merge 1 commit into from

Conversation

seeM
Copy link
Contributor

@seeM seeM commented May 30, 2022

Context

I'm in the process of deep-diving fastai (specifically the tabular sections) and fastcore while on a sabbatical. I'm a big fan so far - it's so user-friendly, and the programming style is reminding me why I got into programming to begin with 😄! While interacting with the notebooks I'm finding tiny opportunities to clean up the implementation and docs.

I hope this is seen as a positive contribution rather than bike shedding. If it doesn't look valuable, please let me know. I'd be more than happy to change my approach and try something else 😃

For interest sake, this PR is a small part of a broader clean up of the dispatch notebook available in my forked master branch (view the full diff here and the updated notebook here). That branch includes more work along these lines, as well as a restructuring of the docs that I found helpful in developing my understanding of the library. I'd be very happy to continue carving out PRs like this from that branch

Description of changes

(commit messages copied as-is)

Make it more dict-like with fewer lines where appropriate:

  • Rename add to __setitem__
  • Add setdefault which cleans up TypeDispatch.add

Also renamed args for consistency: vars referring to keys are named t
since they will always be types, and vars referring to values are named
v (or vs) because they may be functions or nested _TypeDicts.

Make it more dict-like with fewer lines where appropriate:

- Rename `add` to `__setitem__`
- Add `setdefault` which cleans up `TypeDispatch.add`

Also renamed args for consistency: vars referring to keys are named `t`
since they will always be types, and vars referring to values are named
`v` (or `vs`) because they may be functions or nested `_TypeDict`s.
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@seeM
Copy link
Contributor Author

seeM commented Jun 6, 2022

Closing, will focus on #415 instead. cc @jph00

@seeM seeM closed this Jun 6, 2022
@jph00
Copy link
Member

jph00 commented Jun 6, 2022

Closing, will focus on #415 instead. cc @jph00

We might want to reopen this if we end up feeling like plum isn't the best approach.

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