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

Issue #1 JsonResponse can now properly parse control characters #2

Merged
2 commits merged into from
Mar 4, 2014

Conversation

carlosmiranda
Copy link
Contributor

As per RFC 4627 control characters U+0000 to U+001F should be escaped. I created a custom function that does exactly that and used it before invoking createReader.

* @see <a href="http://tools.ietf.org/html/rfc4627">RFC 4627</a>
*/
private String escapeControl(final String input) {
final Pattern pattern = Pattern.compile("[\u0000-\u001f]");
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be a static private field, for the sake of performance

@yegor256
Copy link
Member

yegor256 commented Mar 4, 2014

@carlosmiranda one minor comment, see above

@carlosmiranda
Copy link
Contributor Author

@yegor256 Your comment has been addressed, kindly have another look. :)

@yegor256
Copy link
Member

yegor256 commented Mar 4, 2014

good to merge

@ghost
Copy link

ghost commented Mar 4, 2014

Hey, let me try to merge your branch 1 from carlosmiranda/jcabi-http into branch master of jcabi/jcabi-http. If there won't be any merge conflicts, I'll try to build it. If it builds without errors, I will merge this pull request. I will let you know in any case, in a few...

@ghost
Copy link

ghost commented Mar 4, 2014

I've merged your branch 1 into master of jcabi/jcabi-http without any merge conflicts. Then, I've built the code and found no problems. Thus, this pull request is accepted and will be merged in a few seconds.

$ Crontab `*/5 * * * *`  allowed execution
  SUCCESS 3.8s
$ parallel  allowed with 1 thread(s)
  SUCCESS 25.2s
$ found 1 pull request(s) in Github
  SUCCESS 1.9s
$ 1 out of 1 request(s) approved
  SUCCESS 915ms
$ merging request 2
   ?
$ notified GitHub pull request 2 that merging started
  SUCCESS 10.0s
$ EC2 `m1.small` instance `i-f380b3dd` created in `us-east-1a`
  SUCCESS 7.3s
$ uploaded `.ssh/config`
  SUCCESS 2min
$ uploaded `.ssh/id_rsa`
  SUCCESS 1.5s
$ chmod 600 ~/.ssh/id\_rsa
  SUCCESS 51ms
$ chmod 600 ~/.ssh/config
  SUCCESS 114ms
$ git clone ssh://git@github.com/carlosmiranda/jcabi-http.git code
  SUCCESS 2.1s
$ cd code
  SUCCESS 47ms
$ git remote add dest ssh://git@github.com/jcabi/jcabi-http.git
  SUCCESS 106ms
$ git remote update -p
  SUCCESS 658ms
$ git fetch
  SUCCESS 359ms
$ git checkout -b src origin/1
  SUCCESS 104ms
$ git checkout -b dest dest/master
  SUCCESS 54ms
$ git merge src
  SUCCESS 84ms
$ mvn help:system rultor:steps clean install -Pjcabi -B -C -Pqulice >&2 1> >(col -b | tee -a ../log.txt) 2> >(col -b | tee -a ../log.txt >&2)
  SUCCESS 5min
$ `org.apache.maven.plugins:maven-clean-plugin:clean`
  SUCCESS 581ms
$ `org.codehaus.mojo:jslint-maven-plugin:jslint`
  SUCCESS 9.4s
$ `org.codehaus.mojo:buildnumber-maven-plugin:create`
  SUCCESS 2.9s
$ `org.apache.maven.plugins:maven-resources-plugin:resources`
  SUCCESS 538ms
$ `org.apache.maven.plugins:maven-compiler-plugin:compile`
  SUCCESS 21.5s
$ `com.jcabi:jcabi-maven-plugin:ajc`
  SUCCESS 22.2s
$ `org.apache.maven.plugins:maven-resources-plugin:testResources`
  SUCCESS 63ms
$ `org.apache.maven.plugins:maven-compiler-plugin:testCompile`
  SUCCESS 6.1s
$ `com.jcabi:jcabi-maven-plugin:ajc`
  SUCCESS 4.8s
$ `org.apache.maven.plugins:maven-surefire-plugin:test`
  SUCCESS 33.3s
$ `com.jcabi:jcabi-maven-plugin:versionalize`
  SUCCESS 109ms
$ `org.apache.maven.plugins:maven-jar-plugin:jar`
  SUCCESS 2.2s
$ `org.apache.maven.plugins:maven-failsafe-plugin:integration-test`
  SUCCESS 5.2s
$ `com.qulice:qulice-maven-plugin:check`
  SUCCESS 1min
$ `org.apache.maven.plugins:maven-dependency-plugin:analyze-only`
  SUCCESS 5.0s
$ `org.apache.maven.plugins:maven-dependency-plugin:analyze-dep-mgt`
  SUCCESS 9ms
$ `org.apache.maven.plugins:maven-dependency-plugin:analyze-duplicate`
  SUCCESS 78ms
$ `org.apache.maven.plugins:maven-failsafe-plugin:verify`
  SUCCESS 47ms
$ `com.ning.maven.plugins:maven-duplicate-finder-plugin:check`
  SUCCESS 4.0s
$ `org.codehaus.mojo:xml-maven-plugin:validate`
  SUCCESS 1.6s
$ `org.apache.maven.plugins:maven-install-plugin:install`
  SUCCESS 203ms
$ EC2 instance `i-f380b3dd` terminated
  SUCCESS 454ms
$ EC2 `m1.small` instance in `us-east-1a` costed $0.06
  SUCCESS 3ms
$ com.rultor.guard.github.GhRequest#accept()
   ?

ghost pushed a commit that referenced this pull request Mar 4, 2014
#2: pull request Issue #1 JsonResponse can now properly parse control characters
@ghost ghost merged commit f5953fc into jcabi:master Mar 4, 2014
@dmarkov
Copy link

dmarkov commented Mar 4, 2014

@yegor256 please review this pull request, taking into account these rules

@dmarkov
Copy link

dmarkov commented Mar 4, 2014

@yegor256 There is 15 mins budget allocated for the review

@dmarkov
Copy link

dmarkov commented Mar 4, 2014

@yegor256 I just added 18 mins to your account, many thanks for your contribution..

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants