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

Check file name for non-ascii characters and remove tolower() to ensure file name fidelity in use_test() #613

Merged
merged 7 commits into from Mar 16, 2019

Conversation

@stufield
Copy link
Contributor

@stufield stufield commented Jan 30, 2019

Check file name for non-ascii characters, remove asciify() from slug(), remove tolower() from asciify() to ensure name fidelity, update unit tests accordingly.

Fixes #581

…, remove tolower() from ascify() to ensure name fidelity, update unit tests assordingly.

Fixes r-lib#581
@stufield
Copy link
Contributor Author

@stufield stufield commented Feb 5, 2019

Hi Hadley ... I notice that Travis CI failed one of the tests so I'm not sure if I need to fix anything on my end. I didn't notice anything obvious from the details. Please let me know what (if anything) you need me to do. Thx.

@llrs
Copy link
Contributor

@llrs llrs commented Mar 12, 2019

I think that that error was unrelated to your changes (Although I don't understand why it failed).
Hope this get merged soon

@jennybc
Copy link
Member

@jennybc jennybc commented Mar 12, 2019

@stufield I'll be reviewing PRs soon. What's up with the large commented block here "REMOVE THIS PRIOR TO MERGE"?

@stufield
Copy link
Contributor Author

@stufield stufield commented Mar 12, 2019

Oh geez, this was a while ago, but I believe this was when Hadley and I were going back and forth about how this feature should be implemented. I was unsure if we were going to change our minds, so I was hoping to have an opportunity to "clean up" before submitting a final PR.

Specifically for this section, originally there was an if else branch where if the name was NULL we used the active file name, otherwise we checked (or something like that) ... but during this PR we thought the name should be checked irrespective of whether passed by the user or using the default. Removing the commented section, the if else, you bumped into (sorry for the confusion), would achieve that.

It's been a while, and I'm not sure where things stand at the moment, but would it be easier for me to clean it up and re-submit the PR or for you to do it on your end? Happy to do whatever you prefer.

@jennybc
Copy link
Member

@jennybc jennybc commented Mar 12, 2019

If I see clear path forward when I review it, I'll just make it so (I can push commits here) and we'll get it done. Or I'll leave comments.

@jennybc jennybc merged commit f5cc45a into r-lib:master Mar 16, 2019
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants