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 for issue #162: Replace is() with inherits when class2 is a vector #199

Closed
wants to merge 1 commit into from

Conversation

zeehio
Copy link
Contributor

@zeehio zeehio commented Mar 28, 2022

This pull request fixes failures on R-devel 4.2 as described on:

I basically replace is() with inherits() when class2 is a vector

This is the R source commit where is() became more stringent (January 2022):

Looking forward for a merge, since some other packages I maintain fail because of this error in bioconductor.

Thanks for your time!

Since commit:

- wch/r-source@e0fa9f3

is() gives an error when class2 is a vector. inherits() makes sense in that case
@Max-Bladen Max-Bladen requested review from aljabadi and removed request for aljabadi March 29, 2022 03:01
@Max-Bladen Max-Bladen self-assigned this Mar 29, 2022
@Max-Bladen Max-Bladen linked an issue Mar 29, 2022 that may be closed by this pull request
@Max-Bladen Max-Bladen added wip work-in-progress bug-fix For PR's that address an Issue with `bug` label labels Mar 29, 2022
@zeehio
Copy link
Contributor Author

zeehio commented Mar 29, 2022

The CI is failing in the "Notify slack" stage.

Also I would like to know if there is a timeline for a fix for this issue. The Bioconductor 3.15 freeze is April 11th, in a bit more than two weeks, and if mixOmics can't be ready and with this fix released by then (or before, since I need a bit of time to adapt) I will have to remove it from my package dependencies.

I would rather avoid having to rush at the last minute, that's why I am asking. I can adapt anyway.

@Max-Bladen
Copy link
Collaborator

@aljabadi @lecaolab

This pull request has passed all checks save for the 'Notify Slack' action. This action has not failed on any other PR prior or since this so I don't believe it's an issue to worry about. Please advise as to whether this is ready to merge with master.

@Max-Bladen
Copy link
Collaborator

Hi @zeehio

Once my supervisors see that this is more an error of the GitHub actions rather than your code adjustments, it will be merged and deployed to Bioc. I wouldn't think it would take more than a few days.

Let me know if theres anything else I can do for you

@Max-Bladen Max-Bladen added rapid-review for PRs which will take minimal time to review and close and removed wip work-in-progress labels Mar 29, 2022
@zeehio
Copy link
Contributor Author

zeehio commented Mar 30, 2022

Thanks for offering a fix soon!

There is one more thing you could do :-) Please revert 8fc6d96 since that prevents you from testing on the development version of R and Bioconductor packages, hiding this issue in your CI.

Thanks again!

@Max-Bladen
Copy link
Collaborator

I'll have to wait for @aljabadi to help out in that regard @zeehio. We made changes 8fc6d96 as all our github actions were failing due to using the devel version

@zeehio
Copy link
Contributor Author

zeehio commented Mar 30, 2022

Sure, take your time and feel free to discuss it 👍

In January there was a change in the behaviour of the is() function in R-devel (linked above) where the second argument of is() (named class2) must be a scalar string of length 1 (not a vector). You use a vector in class2 so you have seen that error in some of your issues.

Your R devel builds started to fail by then. The reaction in 8fc6d96 seems to be "R-devel" may have a bug, let's disable it for now (at least the commit and pull request point in that direction) and we will enable it again later. It is reasonable to think that because regressions in R-devel have happened in the past, especially at the beginning of a new development cycle.

In reality the error was because of a breaking change in R-devel, and not a regression there. We are close to the release of R-devel, so that's why I believe the chances of a regression are rather low and the need for testing compatibility with R-devel is now higher.

@zeehio
Copy link
Contributor Author

zeehio commented Apr 4, 2022

Hi again,

Sorry for being pushy. Would it be possible to at least cherry-pick this bugfix on the current bioconductor master branch? Ideally then the the package would build at https://bioconductor.org/packages/devel/bioc/html/mixOmics.html or http://bioconductor.org/checkResults/devel/bioc-LATEST/mixOmics/ and I would be able to avoid the ERRORs in my bioconductor package, where the checks are failing because your package returns the error addressed in this issue.

I understand that you want to push a new version with very interesting new features, but it would be fantastic if until that happens we could at least get mixOmics in Bioconductor-devel into a working state, so packages that depend on it do not ERROR as well.

Beware of the 3.15 release schedule. https://bioconductor.org/developers/release-schedule/ I would rather not have mixOmics and my package deprecated from Bioconductor because of this issue.

If you can't make it, please say so and I will workaround the test or the issue in my package, so I don't get deprecated/removed from Bioconductor.

zeehio added a commit to sipss/AlpsNMR that referenced this pull request Apr 7, 2022
@aljabadi
Copy link
Collaborator

Thanks @zeehio and sorry it took a while to action this, I was a bit late to see this and this is now fixed in the latest GitHub version. It will also be fixed on bioc devel in the next build before the next release.

@aljabadi aljabadi closed this Apr 10, 2022
zeehio added a commit to zeehio/mixOmics that referenced this pull request Apr 11, 2022
@zeehio zeehio mentioned this pull request Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix For PR's that address an Issue with `bug` label rapid-review for PRs which will take minimal time to review and close
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failures on R-devel (4.2)
3 participants