-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
Added :all
Symbol to Javis
#303
Conversation
Codecov Report
@@ Coverage Diff @@
## master #303 +/- ##
==========================================
+ Coverage 95.94% 96.33% +0.39%
==========================================
Files 21 21
Lines 1012 1120 +108
==========================================
+ Hits 971 1079 +108
Misses 41 41
Continue to review full report at Codecov.
|
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.
One minor thing you might want to add and yes please add test cases. I think we actually have some for :same
. Maybe not unit tests but we use it in some animations.
You might wanna have a look @TheCedarPrince |
PR Checklist
If you are contributing to
Javis.jl
, please make sure you are able to check off each item on this list:CHANGELOG.md
with whatever changes/features I added with this PR?Project.toml
+ set an upper bound of the dependency (if applicable)?test
directory (if applicable)?Link to relevant issue(s)
Closes #247
How did you address these issues with this PR? What methods did you use?
I added to the Frames.jl and Object.jl file code to allow for finding the maximum of a background frame. I did not add tests as I saw that we have not added tests for the
:same
keyword. Should we add testing? Does the logic make sense here for what I did?