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

Transition shorthand #46

Merged
merged 2 commits into from
Aug 6, 2020
Merged

Transition shorthand #46

merged 2 commits into from
Aug 6, 2020

Conversation

Wikunia
Copy link
Member

@Wikunia Wikunia commented Aug 6, 2020

As discussed in #33 we decided to revert Translation(p) which mapped to Translation(p, p) and now map it to Translation(O, p)

Additionally one can now write Translation(x, y) such that one does not need to write Point every time.

@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #46 into master will decrease coverage by 0.27%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
- Coverage   90.88%   90.60%   -0.28%     
==========================================
  Files           4        4              
  Lines         329      330       +1     
==========================================
  Hits          299      299              
- Misses         30       31       +1     
Impacted Files Coverage Δ
src/Javis.jl 80.50% <100.00%> (-0.69%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0191ddb...af73171. Read the comment docs.

Comment on lines +133 to 143
Translation(p::Union{Point, Symbol}) = Translation(O, p)

"""
Translation(x::Real, y::Real)

Create a `Translation(O, Point(x,y))` such that a translation is done from the origin.
Shorthand for writing `Translation(Point(x,y))`.
"""
Translation(x::Real, y::Real) = Translation(Point(x, y))

"""
Copy link
Member

Choose a reason for hiding this comment

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

Since we decided to only thoroughly document exported functions, could you add a line of an example invocation of this function? To show how it can be used ?

Comment on lines +162 to 166
Rotation as a transition from 0.0 to `r` .
Can be used as a short-hand.
"""
Rotation(r::Union{Float64, Symbol}) = Rotation(r, r)
Rotation(r::Union{Float64, Symbol}) = Rotation(0.0, r)

Copy link
Member

Choose a reason for hiding this comment

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

Since we decided to only thoroughly document exported functions, could you add a line of an example invocation of this function? To show how it can be used ?

Copy link
Member

@TheCedarPrince TheCedarPrince left a comment

Choose a reason for hiding this comment

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

Looks good. Only a few docstring fixes. Also, can you fix up the docstrings in such a way that they are compatible with Documenter.jl if they are not already prepared?

With regards to unit.jl should we have additional checks there for #18 Or are the animation tests satisfactory?

@Wikunia
Copy link
Member Author

Wikunia commented Aug 6, 2020

Thanks for the review. Can you clarify on what you mean by your point about Documenter? They should all be normal docstrings like others.

I followed docstring practice as for the currently existing docstrings for Transformation and Rotation. I can have a link to javis in all to give an example but I think it's counter productive. I'll write on Slack 😬

I can't think of a unit test for #18 so I think the current tests are ok (mentioned improvements in #38 .

@TheCedarPrince TheCedarPrince self-requested a review August 6, 2020 18:11
Copy link
Member

@TheCedarPrince TheCedarPrince left a comment

Choose a reason for hiding this comment

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

I am approving this now based on our conversation about making the discussion about docstrings into a tutorial.

Add tutorial about rotations and translations (ping #25 )

@Wikunia Wikunia merged commit 4422092 into master Aug 6, 2020
@Wikunia Wikunia deleted the wik-feature-33-1 branch August 6, 2020 19:26
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.

None yet

2 participants