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 local build CMD #331

Merged
merged 5 commits into from
Jul 24, 2023
Merged

Conversation

philipp-baumann
Copy link
Contributor

Hi, thanks for making this great package! While testing local build, I noticed I had to adapt the command for the local build. Hence this mini PR. Cheers, Philipp

@philipp-baumann philipp-baumann changed the title fix build CMD fix local build CMD Jul 22, 2023
@philipp-baumann
Copy link
Contributor Author

The other option would be to do:

cd ..
R CMD INSTALL --no-multiarch --with-keep.source r-polars

@sorhawell
Copy link
Collaborator

sorhawell commented Jul 23, 2023

@philipp-baumann thx alot for spotting this <3

Seem you're right on my local computer and as far as I can see :)

Maybe we should start recommending the Makefile.

@philipp-baumann
Copy link
Contributor Author

@philipp-baumann thx alot for spotting this <3

Seem you're right on my local computer and as far as I can see :)

Maybe we should start recommending the Makefile.

ok cool, no prob.

@philipp-baumann
Copy link
Contributor Author

@sorhawell like the Make approach you suggested.

README.Rmd Outdated Show resolved Hide resolved
Co-authored-by: eitsupi <[email protected]>
@philipp-baumann
Copy link
Contributor Author

thank you @eitsupi

README.md Outdated
@@ -306,7 +306,7 @@ devtools::test() # run all unit tests
**Step 4 (optional):** Build the package locally.

``` r
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you update README.md too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you update README.md too?

sure, sorry for forgetting.

@philipp-baumann
Copy link
Contributor Author

@eitsupi just noticed the same r instead of sh a little bit before in the README.*:

  • Option A: Using devtools.

    Rscript -e 'devtools::install(pkg = ".", dependencies = TRUE)' 

=> do you want a separate PR for this or shall I squash into this PR?

@eitsupi
Copy link
Collaborator

eitsupi commented Jul 24, 2023

@philipp-baumann Thanks for taking a look at that. I'm glad you made the changes in this PR.

Copy link
Collaborator

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

Thanks!

@eitsupi eitsupi merged commit 4b4ce97 into pola-rs:main Jul 24, 2023
@philipp-baumann philipp-baumann deleted the patch-readme-local-build branch July 28, 2023 14:39
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.

3 participants