-
Notifications
You must be signed in to change notification settings - Fork 12
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
changed order_rank_by to order_features_by #120
Conversation
@@ -30,7 +30,7 @@ | |||
#' character will be plotted as colour-code bar. (default: \code{features = | |||
#' NULL}) | |||
#' | |||
#' @param order_rank_by How to order abundance value: By name (\dQuote{name}) | |||
#' @param order_features_by How to order abundance value: By name (\dQuote{name}) |
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.
should this be order_rows_by, or is this coming later in parameter renaming scheme (@TuomasBorman )?
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.
Sorry, I did not see this PR. We are planning to rename this. It is not following the scheme that we are going to use. --> parameter.name
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.
Should we change here now or is that gonna be a separate PR (potentially from @Daenarys8 )?
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.
I think it is better to change these all at once. We have already discussed about this with @Daenarys8
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.
Does this mean that we can merge and close this PR then?
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.
I had now time to check this in detail. There are couple of problems:
- This is not deprecating the old name which means that the might be unexpected behavior if user does not notice this.
- The parameter name should be order.rows.by
So these things should be either addressed before merging, or then other option is to close this PR and make these changes in other PR
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.
I'll close this one and make a new PR, that seems easier.
@Insaynoah what's the status of this one? |
I completely forgot about this one. It should be working. The checks failed because it still tries to run the tests that are currently in use and don't have the updated function name. I updated the test files within the PR and seem to run just fine. |
Hmm still some warnings & errors... |
Updated the parameter name, see issue #86.