-
Notifications
You must be signed in to change notification settings - Fork 16
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
Incorporate reasonable changes to some of the R files within R/ #38
Merged
Merged
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
ec07a2a
Fix typo in comment
Anirban166 9160e91
Changed the working directory inside doc.R so as to suitably run dldo…
Anirban166 0ff30e4
Add an informative message for the error related to wrong set up of w…
Anirban166 9e727aa
Remove the setwd() call to set working directory to docs
Anirban166 9c70572
Remove remaining setwd calls (needs fixes)
Anirban166 7161c14
Fix docdirs file path error (cannot open the connection in file(con, r))
Anirban166 d0ba98f
Fix errors with setwd(docs) removal (dldoc works like before)
Anirban166 0715166
Remove the 2nd setwd() call
Anirban166 60b6e08
Remove unnecessary indentation before ### in R/doc.R
Anirban166 2285c65
Revert back unwanted whitespace changes
Anirban166 dbc4c8d
Revert unwanted whitespace changes
Anirban166 b4782d9
Bring back the setwd() call inside on.exit()
Anirban166 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
error would be better than message
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.
also it would be better to remove the setwd calls, because that can be surprising for the user. can you please refactor and remove setwd?
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.
do you mean the use of
stop()
? or to just throw an error and not use any message? (error = function(e) NULL
)I went with a message because of the 2nd point you mentioned here
This would look like:
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.
Yes, I can remove
setwd()
(guess we won't need this tryCatch then) but then just appending the file path (justdocs/
, as required in our gh-pages branch) to wherever its required would do the job right?(for e.g. the very next line will then be
foot <- filltemplate(foot.info,"docs/templates/foot.html")
?)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.
yes stop() with an error.
yes append file path instead of setwd.
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.
For context, I have two versions of directlabels locally: One is with the current gh-pages branch on D: (where I test dldoc)
and the other is my master branch with this PR's changes on E: (from where I install dldoc by source)
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.
I don't know. Can you please debug? what file is it? insert print() and browser() statements?
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.
Sure, and sorry for not doing so earlier - debugging otw!
For the previous error, the docdirs variable (which would be the list of strings of the names of different plot type directories under tests/doc) was empty as because I forgot to append the file path to it
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.
Hi @tdhock is it okay if I just keep the first
setwd()
call? (removed the other two, and dldoc works like usual up till this point)It is really convenient, and I mean otherwise I'm just editing the file paths but due to that single removal, a lot of places are affected and it gets a bit hard to debug, for e.g. the one that I showed above was possibly occuring because the tests/doc/lineplot/lars.R file couldn't access the Rdata file (called via
extract.plot
). This was what the last version (modified a week back) looked like:Sorry this took long, I was out of commission for a few days following the aftermath from my vaccination shot (right arm was completely sore and my body felt weak) and I couldn't do any GSoC work :(
If it is okay, could you please merge this for now? (Also #41 )
I want to start work on the third point now, and I'll give this a re-visit later (also the 2nd workflow is done with the changes, but need to modify dldoc before I push that onto master)
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.
ok that is fine to keep a setwd for now but it would be good to remove them eventually.