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

2 unrelated improvements: 1. base_pkgs() can "hang". 2. isFALSE() is faster and better in base. #66

Merged
merged 4 commits into from May 20, 2022

Conversation

mmaechler
Copy link
Contributor

@mmaechler mmaechler commented May 20, 2022

  1. I found that knitr::write_bib() would hang the R process for me. Found then that it was when xfun::base_pkgs() was called.
    The reason is I have 20'000 packages installed in my .libPaths(). Instead, the base packages are always in .Library hence 197e357.

  2. I saw your isFALSE(). Presumably it is older than the one in {base} which is also a few years old. When we introduced it (I was involved with it), we found we do not want identical, because a FALSE (or TRUE) can also end up with names or other such attributes. The change cfbf040 keeps xfun's isFALSE() local, i.e, does just not export it. Personally I think you should even not define it anymore, but rather use the base one for consistency.

@CLAassistant
Copy link

CLAassistant commented May 20, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Owner

@yihui yihui left a comment

Choose a reason for hiding this comment

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

For 1, that makes perfect sense! For 2, I wish to remove it, too, but 1) I need to figure out which reverse dependencies will be affected and fix them first (like this); then deprecate this function to notify users for some time instead of suddenly removing it; 2) base::isFALSE() requires R >= 3.5.0, and currently we are supporting R >= 3.3.0, so it will take some time before I can consider removing isFALSE from xfun (yes, it has existed before R 3.5.0 was released, and I think base::isFALSE() makes more sense).

Thanks!

@yihui
Copy link
Owner

yihui commented May 20, 2022

For isFALSE(), I'll deal with it in a separate PR, since this is not a trivial decision to make like the one on using .Library (it's better to send separate patches in separate PRs so that the easier ones can be quickly merged first).

@yihui yihui merged commit 20a3147 into yihui:main May 20, 2022
@yihui yihui mentioned this pull request May 20, 2022
@mmaechler
Copy link
Contributor Author

... (it's better to send separate patches in separate PRs so that the easier ones can be quickly merged first).

Yes, I'm sorry for the extra work you had.
{Somehow the github web interface allowed me to do one PR from the two changes in my clone (mmaechler/xfun which was "ahead of" yours by 2 commits) easily, and I tried a while to do separate PRs instead, but did not see how ..

@yihui
Copy link
Owner

yihui commented May 23, 2022

No worries at all! I really appreciate your contribution!

If you make the changes in separate branches, you can then submit a separate PR for each branch. For the Github web interface, you can click on the edit button on the toolbar of a file (open a file on Github and find the pencil button on the right) to suggest changes:

the edit button

Then the web interface will guide you to create a new branch for the changes that you made.

I guess that you did was first forking the repo, then making the changes in the main branch, and finally submitting the PR. All changes were in the same branch, so you could only submit one PR.

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.

None yet

3 participants