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

Fix 143 #144

Merged
merged 21 commits into from
Nov 14, 2024
Merged

Fix 143 #144

merged 21 commits into from
Nov 14, 2024

Conversation

mvarewyck
Copy link
Collaborator

@mvarewyck mvarewyck commented Nov 4, 2024

This PR fixes

I have increased version to 2.3.2 (not sure what are the internal guidelines about this)

@mvarewyck mvarewyck self-assigned this Nov 4, 2024
Copy link
Collaborator

@damianooldoni damianooldoni left a comment

Choose a reason for hiding this comment

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

Thanks @mvarewyck for the improvements of the function!
The changes as they are now are breaking code. And it means that we need to upgrade to version 3.0.0. Typically, I would reply that such changes are not worth a major version change. However, this package is not on CRAN and not widely used. So, I could say that we can upgrade to 3.0.0.

However, please consider the following changes:

  • make old arguments (relative) deprecated. Set internally response_type based on the value given to relative. Please, use lifecycle::deprecated() for it. Quite self-explanatory example from camtraptor::read_camtrap_dp()
  • As we go for 3.0.0, it's definitely time to start a NEWS.md from now on. Could you please add such file in root folder? Don't worry about older versions. We will start from 3.0.0. You can follow this example from camtraptor which is automatically rendered as this website.

@mvarewyck
Copy link
Collaborator Author

This PR will fix #140

@damianooldoni
Copy link
Collaborator

Hi @mvarewyck 👋
Is it ok if I review this PR tomorrow morning?

@mvarewyck
Copy link
Collaborator Author

Hi @mvarewyck 👋 Is it ok if I review this PR tomorrow morning?

Sure, ready from my side.
There are still some notes in the R CMD check, but they are related to existing code, so I ignored them.

Remove `@author` field in documentation of help function.
Avoid using @importFrom where possible
- Add dot at the end of each sentence.
- Use Uppercase for first word after brackets ( )
pkgname::function_name syntax is enough.
This should be done in all other files as it improves readability.
Copy link
Collaborator

@damianooldoni damianooldoni left a comment

Choose a reason for hiding this comment

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

Very nice work 👍 Thanks for adding value to this package.

I have updated your role within the package: you are an author 🎉 And so I removed the @author field in roxygen documentation of the help function. Otherwise I should do the same for all other functions where others have worked on as well. git commits history is enough to know who did what in the rare case I need to ask about the code.

For the rest, I pushed some minor changes. The commit titles should be self explicative.
If you have any question about them, please let me know.

@mvarewyck mvarewyck merged commit 7e0f1fb into main Nov 14, 2024
6 checks passed
@damianooldoni damianooldoni deleted the fix_143 branch November 14, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants