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

Addition of User-Friendly functions. #6

Merged
merged 13 commits into from
Feb 4, 2021

Conversation

BradyAJohnston
Copy link
Collaborator

Creation of user friendly functions for addition of styles, and selection algebra. Provides options for user to choose from, rather than having to lookup the documentation.

m_cartoon_style()
m_stick_style()
m_sphere_style()
m_label_style()
m_line_style()
m_sel()

Also creation of functions load bio3d structures and fetch from the pdb using bio3d as the intermediate.

m_bio3d()
m_fetch_pdb()

Slight tweak to the r3dmol(). Addition of viewer_spec argument, which is passed through into the configs for in addition to any ... arguments supplied. Allows for prompting of common useful options such as cartoonQaulity and nomouse.

 # forward options using x
x <- list(
id = id,
configs = c(list(...), viewer_spec)

Copy link
Owner

@swsoyee swsoyee left a comment

Choose a reason for hiding this comment

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

A good contribution! Although I have left some comment, may I edit this PR and make some code formatting fix to pass the checking problem before reviewing the main part of implements? And I will write some message and description in each commit, so it's quite convenient for your PR next time.
I must apologize that I should write some standard about how to make a contribution in wiki, but r3dmol just a small project and in it's very early stage.

R/style.R Outdated Show resolved Hide resolved
R/style.R Outdated Show resolved Hide resolved
R/style.R Outdated Show resolved Hide resolved
R/style.R Outdated Show resolved Hide resolved
R/style.R Outdated Show resolved Hide resolved
R/style.R Outdated Show resolved Hide resolved
R/style.R Outdated Show resolved Hide resolved
renv.lock Show resolved Hide resolved
@swsoyee swsoyee marked this pull request as draft January 27, 2021 05:50
@BradyAJohnston
Copy link
Collaborator Author

I've gone through and made changes and fixed things that were immediately needing to be addressed.

I also made a change to the m_*_style() functions which are now m_style_*() which will make it easier for people who don't know what styles are available.

What should I do about the renv stuff? Should I copy over the files from your repo and do a new pull request? Unsure how to resolve this.

@swsoyee
Copy link
Owner

swsoyee commented Jan 28, 2021

@BradyAJohnston It's seems very good after you change those function to m_style_*(), and with styler the codes are more comfortable now : )

What should I do about the renv stuff? Should I copy over the files from your repo and do a new pull request? Unsure how to resolve this.

When, it's fine so just keep going in this PR. There are also some suggestions if you know how to fix/implements:

  1. Would you mind use devtools::check() to check the problem and fix the errors/warnings/notes?
  2. Add test to https://github.com/swsoyee/r3dmol/tree/master/tests/testthat (Those functions which I created also need to
    add enough test to verify them, but I couldn't find a best practice to implement due to they connecting the R side and JavaScript side. I will keep finding some good way to do the test).
  3. You could add yourself into the https://github.com/swsoyee/r3dmol/blob/master/DESCRIPTION#L5-L8 as a contributor if you like.
  4. You could add those functions into pkgdown site (https://github.com/swsoyee/r3dmol/blob/master/_pkgdown.yml) and bring your article to the the official document site (if you like).

I'm sorry for those requests, if you think it's too heavy or do not know how to deal with those questions please let me know.

@BradyAJohnston BradyAJohnston marked this pull request as ready for review January 29, 2021 00:57
@swsoyee
Copy link
Owner

swsoyee commented Jan 29, 2021

Thank you for your contribution! I will check the code this weekend!

@BradyAJohnston
Copy link
Collaborator Author

I've changed up the m_add_sphere/arrow/cylinder() functions to work with m_shape_spec() and prompt for some inputs as well. I plan on doing the same for the m_add_*() as well, but I will revisit that some time next week after you've had a chance to look through what I've done so far.

So far I've been continuing the m_* nomenclature, but unsure if that should apply to everything in the package? Something to think about.

Copy link
Owner

@swsoyee swsoyee left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution again! I have check this changes and I think it's highly acceptable.
Some minor changes requested and I only write the comments in R/view_options.R, other functions could address those comments and make some minor fix.

So far I've been continuing the m_* nomenclature, but unsure if that should apply to everything in the package? Something to think about.

When call the functions, many users may not going to use r3dmol::function() but only type function(). If we named the function in a commen words that may confilict to other package, it may cause some unexpectful errors, and the user may not be aware of that.
Further more, if the user library(r3dmol) at first, just type m_ and the all functions which are available in {r3dmol} will be listed out for a quick searching. It could be tedious in typing lots of m_, but the user will definitely know the functions come from {r3dmol}.

vignettes/stylefunctions.Rmd Outdated Show resolved Hide resolved
vignettes/stylefunctions.Rmd Outdated Show resolved Hide resolved
vignettes/stylefunctions.Rmd Outdated Show resolved Hide resolved
vignettes/stylefunctions.Rmd Outdated Show resolved Hide resolved
vignettes/stylefunctions.Rmd Outdated Show resolved Hide resolved
R/view_options.R Outdated Show resolved Hide resolved
R/view_options.R Outdated Show resolved Hide resolved
R/view_options.R Outdated Show resolved Hide resolved
}

m_viewer_spec
}
Copy link
Owner

Choose a reason for hiding this comment

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

A test for list converting is not difficult so adding a test would be prefer.
https://testthat.r-lib.org/index.html

R/view_options.R Outdated Show resolved Hide resolved
@BradyAJohnston
Copy link
Collaborator Author

I've gone through and cleaned up everything you mentioned, except for the creation of testing. I've added that as an issue #7 .

@swsoyee
Copy link
Owner

swsoyee commented Feb 2, 2021

Thank you! I will check them tonight👍

Copy link
Owner

@swsoyee swsoyee left a comment

Choose a reason for hiding this comment

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

I'm sorry for the late response, just a minor comments 👍
Feel free to ask any question.

R/utils.R Outdated Show resolved Hide resolved
vignettes/style-functions.Rmd Outdated Show resolved Hide resolved
vignettes/style-functions.Rmd Outdated Show resolved Hide resolved
R/style.R Show resolved Hide resolved
R/add.R Outdated Show resolved Hide resolved
R/sel.R Outdated Show resolved Hide resolved
R/set.R Outdated Show resolved Hide resolved
Re-write all `style = ` in examples.
@BradyAJohnston
Copy link
Collaborator Author

I think I've addressed everything in your latest set of comments. Thanks so much for all of the feedback!

Copy link
Owner

@swsoyee swsoyee left a comment

Choose a reason for hiding this comment

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

Everything looks very good! Finally, it would be better to use

styler:::style_active_pkg()

to style all the code at once. 😉

R/add.R Outdated Show resolved Hide resolved
@swsoyee swsoyee linked an issue Feb 4, 2021 that may be closed by this pull request
Copy link
Owner

@swsoyee swsoyee left a comment

Choose a reason for hiding this comment

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

Good! I merge it and thank you for your contribution!

@swsoyee swsoyee merged commit b2d0393 into swsoyee:master Feb 4, 2021
@swsoyee
Copy link
Owner

swsoyee commented Feb 4, 2021

@BradyAJohnston Could you please have to check the code again because the the style seems not be properly set.
https://swsoyee.github.io/r3dmol/articles/r3dmol.html

@swsoyee swsoyee mentioned this pull request Feb 18, 2021
87 tasks
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.

User-Friendly Functions
2 participants