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.

Sign up for GitHub

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

Jump to bottom

refactor: Remove marshmallow extraction scripting, move docstrings into marshmallow field classes. #1961

Merged
merged 19 commits into from
Apr 26, 2022
Merged

refactor: Remove marshmallow extraction scripting, move docstrings into marshmallow field classes. #1961

merged 19 commits into from
Apr 26, 2022

Conversation

ksbrar
Copy link
Collaborator

@ksbrar ksbrar commented Apr 25, 2022 •
edited
Loading

Removes marshmallow extraction entirely, instead we'll insert docstrings directly into marshmallow fields.

Tradeoff: no more editor introspection without duplicating docstrings, but no more extraction/scripting pains either.

@ksbrar ksbrar added bug Something isn't working feature New feature or request productivity and code quality Engineer productivity, maintainability, consistency, readability labels Apr 25, 2022
@ksbrar ksbrar self-assigned this Apr 25, 2022
@github-actions
Copy link

github-actions bot commented Apr 25, 2022 •
edited
Loading

Unit Test Results

       6 files  ±0         6 suites  ±0   1h 22m 56s ⏱️ - 13m 8s
2 761 tests  - 2  2 730 ✔️ +10  31 💤 ±0  0 ❌  - 12 
8 283 runs   - 6  8 185 ✔️ +  6  98 💤 ±0  0 ❌  - 12 

Results for commit 93b340c. ± Comparison against base commit 59cf85d.

♻️ This comment has been updated with latest results.

justinxzhao
justinxzhao approved these changes Apr 25, 2022
View reviewed changes
Copy link
Collaborator

@justinxzhao justinxzhao left a comment

Choose a reason for hiding this comment

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

LGTM! Passing in descriptions as strings directly seems like a great improvement over using docstrings and having to parse them ourselves.

@ksbrar ksbrar mentioned this pull request Apr 26, 2022
Merged
@tgaddair tgaddair merged commit 41323c3 into ludwig-ai:master Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgaddair tgaddair tgaddair approved these changes

@justinxzhao justinxzhao justinxzhao approved these changes

@w4nderlust w4nderlust Awaiting requested review from w4nderlust w4nderlust is a code owner

Assignees

@ksbrar ksbrar

Labels
bug Something isn't working feature New feature or request productivity and code quality Engineer productivity, maintainability, consistency, readability
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ksbrar @tgaddair @justinxzhao