-
Notifications
You must be signed in to change notification settings - Fork 8
gt has changed the default table position from !t
to t
#529
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
Conversation
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.
Thanks, @yihui!
@yihui I think we can agree on the problem (upstream dependencies make breaking changes that will break downstream packages tests easily), but I respectfully disagree with the recommendation on not using The rationale has been explained in #371, but maybe it can use more clarity: having to make frequent CRAN updates due to upstream dependency changes adds unnecessary time and efforts burden to maintainers (see previous examples: Merck/simtrial#234, keaven/gsDesign#115). Although we welcome gt authors to send PRs to fix the problems, it is us who still have to make CRAN releases, and you know that is not as trivial as releasing a Python package to PyPI due to the high standards. We also don't want to be a potential blocker for gt's CRAN releases. |
@nanxstats Thanks for the explanation! I understand it better now. However, I think it's actually a great thing for us to be a potential blocker for the CRAN releases of packages that we depend on (sometimes someone has to be "that guy" :). That's the whole point of CRAN requiring revdep checks---upstream packages can't easily make arbitrary changes without letting downstream packages know. I guess we have no disagreement on this. The only thing to discuss is the potential trouble for us to have to make a new CRAN release before upstream packages bring their breaking changes to CRAN. I wouldn't worry too much about it for two reasons:
I think the potential burden you mentioned would be worthwhile, as it offers us better guarantee on whether our package does what we expect it to at any time. If we passively wait for upstream changes, there might be a (short) time period in which our package "works" on CRAN but breaks in practice. I may well be paranoid here, but just want to point out the possibility and cost. I don't really have a strong opinion on this, so it's totally fine to leave it behind. |
@yihui I actually agree with everything you said. I guess our optimization objective has been inexplicitly set to minimize maintainer time and effrot investment, instead of "doing the right thing" and "be that guy". In our reality, people have other business priorities and can't do this full time. Previously, I suggested the team to adopt a practice of making more regular CRAN releases unconditionally (like once a month or so) but failed for similar reasons. So to me, this is more of a social issue instead of a technical issue. |
Totally understood! |
cf rstudio/gt#1935
Due to our use of
skip_on_cran()
intests/testthat/test-independent_as_gt.R
, the gt maintainer failed to detect this breakage in our package before the new CRAN release of gt. I'd strongly recommend that we reserve the use ofskip_on_cran()
only for the absolute necessary cases (e.g., complicated dependencies that CRAN may not have). The skip was from the PR #371 by @nanxstats but I don't quite understand the rationale. If changes in gt break our tests, gt should be responsible for the problem, not us. In fact, gt won't be able to make the new release to CRAN before fixing the problem with us, so we shouldn't have any problem with the existing CRAN release of our package.