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

Testing 6 #17

Closed
wants to merge 24 commits into from
Closed

Testing 6 #17

wants to merge 24 commits into from

Conversation

rahul799
Copy link
Collaborator

This PR will test
Different mixes of R vs other files, including confirming that it does nothing when no R files are changed

@rahul799
Copy link
Collaborator Author

/style

@infotroph
Copy link
Owner

infotroph commented Jun 26, 2020

The next invocation of the style command will find only .prettierignore changed. Since no R files have changed, there's nothing for styler to do. Let's see what happens!

@infotroph
Copy link
Owner

/style

@infotroph
Copy link
Owner

Answer, styler runs, finds no changes to make, and exits reporting success. Nothing is committed.

The only downside I see here is that you need to go check the actions tab to tell whether the run is still in progress or completed without finding any changes. But then again most people won't be requesting a styler run when there's nothing to style, so this is probably not a big deal.

@infotroph
Copy link
Owner

The next invocation of the styler command will find no R files changed, but should notice and fix one misedited Rd file. Let's see what happens!

@infotroph
Copy link
Owner

/style

@infotroph
Copy link
Owner

Answer: Styler runs but doesn't commit the change! It took me a while to track down why:

  • Issue 1: Initial list of changed files includes the changed base/utils/PEcAn.Rd, but only *.R and *.Rmd are passed in to files_to_style.txt. files_to_style.txt is the only artifact passed to the check step, so the check step never gets the information that base/utils needs to be Roxygenized. To fix this, the check step should be given the full list of changed files.

  • Issue 2: But In this case, the process of checking models/fates and models/gday prompts Make to re-Roxygenize base/utils anyway! But the change isn't committed because the command

    if [[ $(git diff --name-only --cached) != "" ]]; then  git commit -m 'automated documentation update' ; fi
    

    returns [[: not found. To fix this, we should either use [ instead of the bashism [[, or explicitly execute the command in bash rather than the default shell.

@infotroph
Copy link
Owner

The next invocation tests again after fixing issue 2 ([[: not found) from above. If it's fixed correctly, the next commit should revert the changes to PEcAn.Rd.

Note also that each time we invoke styler it looks at the set of all files changed in this pull request, not just the files changed in the most recent commit like I was assuming above. But of course the style changes to *.R have already been committed by previous styler invocations, so we should only see changes in the newly touched files.

@infotroph
Copy link
Owner

/style

@infotroph
Copy link
Owner

/style

(Apparently I didn't actually push the [[ fix before)

@infotroph
Copy link
Owner

/style

@infotroph
Copy link
Owner

Result: In this case the mangled Rd file gets fixed, but by accident (because utils gets rebuilt when make .doc/models/fates runs) rather than because of the change. This should be fixed eventually, but "change to Rd file with no corresponding change to R files in the same package" is a weird enough corner case that it needn't stop us from merging this now.

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

2 participants