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 methods for working with enums. #491

Merged
merged 7 commits into from
Mar 25, 2021
Merged

Added methods for working with enums. #491

merged 7 commits into from
Mar 25, 2021

Conversation

PavelBal
Copy link
Member

No description provided.

@PavelBal PavelBal mentioned this pull request Mar 24, 2021
#' myEnum <- enum(c(a = "b"))
#' myEnum <- enumPut("c", "d", myEnum)
#' myEnum <- enumPut(c("c", "d", "g"), c(12, 2, "a"), myEnum, overwrite = TRUE)
enumPut <- function(keys, values, enum, overwrite = FALSE) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Allows to extend existing enums

@PavelBal PavelBal requested a review from msevestre March 24, 2021 11:16
@msevestre
Copy link
Member

@PavelBal CAn you add a test file for this? The behavior is starting to justify it :)

R/enum.R Show resolved Hide resolved
#'
#' @return Enum without the removed entries
#' @export
enumRemove <- function(keys, enum) {
Copy link
Member

Choose a reason for hiding this comment

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

woudl it work with keys being ONE value and not an array?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yepp.

Copy link
Member

Choose a reason for hiding this comment

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

So why do we always do sthg like this in ospsuite-r

quantities<- c(quantities) to ensure that we have a list

Copy link
Member Author

Choose a reason for hiding this comment

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

I think... Because a quantity (or any R6-object) by themselves are kind of a list, and trying to iterate over an R6-object produces some mess... Having an object as enum-key would not work.

Copy link
Member

Choose a reason for hiding this comment

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

oh yeah that's right. Thanks brother

@PavelBal
Copy link
Member Author

@msevestre I added some tests.

@msevestre
Copy link
Member

@PavelBal Build is failing

@msevestre
Copy link
Member

oh yeah it's failing because I need to merge the other branch first. then if you can merge/rebase develop into your branch, we should be good to go

@PavelBal
Copy link
Member Author

@msevestre rebased

@msevestre
Copy link
Member

looks good. I'll merge when the build is finished

@msevestre
Copy link
Member

@PavelBal You need to create thew new documentation file (CTRL+SHIFT+D )to update the documentation. Otheriwse it won't be vibisle

@PavelBal
Copy link
Member Author

Oh, I thought it's re-created on build... Added!

@msevestre
Copy link
Member

still one example that is being run...pain in the butt.
https://ci.appveyor.com/project/open-systems-pharmacology-ci/ospsuite-r/builds/38385449

You probably need to regenerate the documentation so that it is being updated.

@PavelBal
Copy link
Member Author

@msevestre Actually, I just realized... I expose all those methods, but we do not expose enum itself (also this is why examples are failing). I think we should either export everything or nothing. I would prefer exporting enum so it can be used by user. What do you think?

@msevestre
Copy link
Member

This was an issue because enum is used in TLF and other Lib and we were getting conflicts. That why we didn't export. Yes you can still use it from the outside?

@PavelBal
Copy link
Member Author

Yes you can still use it from the outside?

You cannot create a new one:

> library(ospsuite)
> myEnum <- enum(c("a", "b"))
Error in enum(c("a", "b")) : could not find function "enum"

@msevestre
Copy link
Member

What we can try to do is
expor the one in ospsuite.core
Do not create one at all in ReportingEngine and use this one
Rename the one in tlf.
For some reason, I think this was not working

Anyway, in the meantime, can you put the dontrunn flag or sthg like this in the example so that the build passes?

@msevestre msevestre merged commit f157010 into develop Mar 25, 2021
@msevestre msevestre deleted the enum-extensions branch March 25, 2021 13:08
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.

2 participants