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

Added a short help file for class Pdiff #4762

Open
wants to merge 3 commits into
base: develop
from

Conversation

@mhampton
Copy link

mhampton commented Feb 9, 2020

Purpose and Motivation

Types of changes

  • Documentation
  • Bug fix
  • New feature
  • Breaking change

To-do list

  • Code is tested
  • All tests are passing
  • Updated documentation
  • This PR is ready for review
@@ -0,0 +1,17 @@
class:: Pdiff
summary:: returns the difference between the current and previous values of an enclosed pattern
related:: Classes/Padd

This comment has been minimized.

Copy link
@jamshark70

jamshark70 Feb 9, 2020

Contributor

Padd is not really related (different purpose).

The inverse operation of differentiation is integration. It isn't obvious at first, but in pattern-land, the integrator is Pseries. So I'd put this one in as the related class.

This comment has been minimized.

Copy link
@mhampton

mhampton Feb 9, 2020

Author

Thanks! That makes a lot of sense, I have changed it. I also added a file for Pdrop.

@jamshark70

This comment has been minimized.

Copy link
Contributor

jamshark70 commented Feb 11, 2020

That makes a lot of sense, I have changed it. I also added a file for Pdrop.

I don't see the change (Padd --> Pseries) registered in the new commit. Could you check again?

Ideally, method documentation (*new) should list the arguments too. Pdiff and Pdrop are fairly simple so I won't be too picky, but Pdrop would benefit from a couple of argument:: tags.

For procedure, usually we want to avoid creeping scope in PRs -- add some help files, let them be reviewed and merged. Other help files can go into a different PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.