Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

Alternate fix for does #247

Merged
merged 4 commits into from
Dec 18, 2022
Merged

Alternate fix for does #247

merged 4 commits into from
Dec 18, 2022

Conversation

elijahbenizzy
Copy link
Collaborator

@elijahbenizzy elijahbenizzy commented Dec 14, 2022 •
edited by skrawcz
Loading

[Short description explaining the high-level reason for the pull request]

Changes

How I tested this

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

@skrawcz
Fixes does function wrapping #244
208990a 
In issue #244 types were not being propagated of
the function being wrapped.

This changes that by adding the functools.wraps
decorator.

Adds a unit test to test for two decorators playing nice
together and verified that it's broken before and fixed
afterwards.
@skrawcz
Removes print statement
cdea1f8
@elijahbenizzy
Cleaner fix for @does. The function is only useful for the
0f8ae0d 
node generator -- not for the rest. This ensures that we don't use it
to handle signatures, rather, we just use the node's return types.
@elijahbenizzy
Copy link
Collaborator Author

When we rebase let's squash and rebase cause this also has the other fix

@elijahbenizzy
Copy link
Collaborator Author

Also will have to rebase on top of this... #237, which is going to be a mess.

@skrawcz
Copy link
Collaborator

skrawcz commented Dec 15, 2022

@elijahbenizzy we should add unit tests for the other decorators you changed to verify the before and after :)

@skrawcz
Adds unit test to cover extract_fields with does
c0343d8 
Adds a test to cover Elijah's changes.

In the process I created #249.
Since I think we need to add more unit tests around decorator interactions.
@skrawcz skrawcz mentioned this pull request Dec 18, 2022
Closed
7 tasks
@skrawcz skrawcz linked an issue Dec 18, 2022 that may be closed by this pull request
Cannot use @does with @extract_columns #244
Closed
skrawcz
skrawcz approved these changes Dec 18, 2022
View reviewed changes
Copy link
Collaborator

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

Works for me.

@skrawcz skrawcz merged commit 9babf7e into main Dec 18, 2022
@skrawcz skrawcz deleted the alternate-fix-for-does branch December 18, 2022 05:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@skrawcz skrawcz skrawcz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot use @does with @extract_columns
2 participants
@elijahbenizzy @skrawcz