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

Replace deprecated stringByReplacingPercentEscapesUsingEncoding: with stringByAddingPercentEncodingWithAllowedCharacters: #19792

Closed

Conversation

pvinis
Copy link
Contributor

@pvinis pvinis commented Jun 18, 2018

Replace some NSString deprecated methods.
motivation for these prs is less warnings reported on xcode everytime we compile a rn app.

Test Plan

N/A

Release Notes

[INTERNAL] [DEPRECATIONS] [NSString] - Replace NSString deprecation methods.

@pvinis pvinis requested a review from shergin as a code owner June 18, 2018 20:35
@pull-bot
Copy link

pull-bot commented Jun 18, 2018

Warnings
⚠️

📋 Changelog - This PR appears to be missing Changelog. Please add a section called "changelog" and format it as explained in the contributing guidelines.

Generated by 🚫 dangerJS

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 18, 2018
@react-native-bot react-native-bot added ✅Test Plan Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Jun 18, 2018
@janicduplessis
Copy link
Contributor

Looks good! Thanks for the fixes :)

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jun 19, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@janicduplessis is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

@facebook-github-bot facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Jun 20, 2018
@pvinis
Copy link
Contributor Author

pvinis commented Jun 20, 2018

hmm.. tests here pass. would be nice to see what the problematic internal test is. if it's not sensitive, maybe we could open source it? any ideas what could it be @janicduplessis ?

@janicduplessis
Copy link
Contributor

Sorry about the delay, it failed to import into the internal fb repo, I'll try landing again and if it doesn't work I'll ping someone at fb.

@janicduplessis
Copy link
Contributor

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jul 15, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@janicduplessis is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@hramos hramos added ✅Changelog and removed Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Jan 15, 2019
@cpojer
Copy link
Contributor

cpojer commented Jan 24, 2019

Let's try this again!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@cpojer
Copy link
Contributor

cpojer commented Jan 25, 2019

Just getting back to this. Thanks for submitting this PR. Unfortunately it seems that unit tests are failing:

Failure: ((result.absoluteString) equal to (@"http://example.com/blah%20blah/foo")) failed: ("http%3A%2F%2Fexample.com%2Fblah%20blah%2Ffoo") is not equal to ("http://example.com/blah%20blah/foo")
Path: react-native/RNTester/RNTesterUnitTests/RCTConvert_NSURLTests.m
Line Number: 54

This is the file specifically: https://github.com/facebook/react-native/blob/master/RNTester/RNTesterUnitTests/RCTConvert_NSURLTests.m

Would you mind doing another pass and fixing those issues, then we can finally land it? Again, sorry for the long wait on this :(

@pvinis
Copy link
Contributor Author

pvinis commented Jan 25, 2019

I'll try it now and get back to you.

@pvinis pvinis force-pushed the fix/replace-deprecated-string-method branch from 761f762 to 5cab758 Compare January 25, 2019 11:27
@pvinis
Copy link
Contributor Author

pvinis commented Jan 25, 2019

This was a good catch actually. I changed the allowed characters to be the ones they should be for each case, ran all the tests and now all is well here.

I was thinking of putting the character set into a category of NSCharacterSet but I don't know if you want that or if there is a place for categories etc. Let me know what you think, but I'm happy another one of my PRs is soon in the core code :D

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

@pvinis merged commit 61ca119 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Jan 25, 2019
@react-native-bot react-native-bot added the Merged This PR has been merged. label Jan 25, 2019
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. Import Failed labels Feb 6, 2019
matt-oakes pushed a commit to matt-oakes/react-native that referenced this pull request Feb 7, 2019
…th `stringByAddingPercentEncodingWithAllowedCharacters:` (facebook#19792)

Summary:
Replace some NSString deprecated methods.
motivation for these prs is less warnings reported on xcode everytime we compile a rn app.

N/A

[INTERNAL] [DEPRECATIONS] [NSString] - Replace NSString deprecation methods.
Pull Request resolved: facebook#19792

Differential Revision: D8515136

Pulled By: cpojer

fbshipit-source-id: 4379ef4e229ef201685b87e54ac859ba3d30a833
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants