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

Modifying whitespace removal for logging #460

Closed
wants to merge 1 commit into from
Closed

Modifying whitespace removal for logging #460

wants to merge 1 commit into from

Conversation

szgabsz91
Copy link

This pull request is for issue #459

…uotation marks and apostrophes remains intact
@harawata
Copy link
Member

Thank you for your contribution, but the regular expression throws java.lang.StackOverflowError if the sample-query.txt contains a large query (about 1300+ characters in my test).
It is most likely caused by this known java issue.

In case you are trying to rewrite the method, please make sure it is efficient in memory/cpu usage.
This method is called many times during runtime and the string parameter could be large.

p.s.
Please put resource files like .txt in the same directory as the java source files.
I know it is against the maven standard, but that's how this project manages test resources ;D

Thanks again for your time!
Iwao

@harawata harawata closed this Aug 11, 2015
@szgabsz91
Copy link
Author

Aha. OK, anyways if I have a better solution, I'll send a new one. This is not a major issue, but very confusing for the first time. Thanks!

@hazendaz
Copy link
Member

Sounds like we need to comply with maven if any of us is ever inclined and have time ;) maybe note that as an issue at least and I'll look at doing that in one go sometime this year.

Sent by Outlook for Android

On Tue, Aug 11, 2015 at 7:01 AM -0700, "Iwao AVE!" notifications@github.com wrote:
Thank you for your contribution, but the regular expression throws java.lang.StackOverflowError if the sample-query.txt contains a large query (about 1300+ characters in my test).
It is most likely caused by this known java issue.

In case you are trying to rewrite the method, please make sure it is efficient in memory/cpu usage.
This method is called many times during runtime and the string parameter could be large.

p.s.
Please put resource files like .txt in the same directory as the java source files.
I know it is against the maven standard, but that's how this project manages test resources ;D

Thanks again for your time!
Iwao


Reply to this email directly or view it on GitHub:
#460 (comment)

@harawata
Copy link
Member

Hi @hazendaz ,

Regarding the directory structure, there was a discussion on the dev list.
Following the standard is a good thing for sure, but the current structure has some pros as well.

  • It is easier for contributors to create a new test case from the BaseTest (just copy one directory and you're good to go).
  • It is also easier for us committers to review existing test cases (no need to switch between src/resources folder to review java/xml mappers).

I would suggest you to post your arguments for the migration to the dev list and see what others think.

@hazendaz
Copy link
Member

Sounds good. Thanks

Sent by Outlook for Android

On Wed, Aug 12, 2015 at 6:17 PM -0700, "Iwao AVE!" notifications@github.com wrote:
Hi @hazendaz ,

Regarding the directory structure, there was a discussion on the dev list.
Following the standard is a good thing for sure, but the current structure has some pros as well.

  • It is easier for contributors to create a new test case from the BaseTest (just copy one directory and you're good to go).
  • It is also easier for us committers to review existing test cases (no need to switch between src/resources folder to review java/xml mappers).

I would suggest you to post your arguments for the migration to the dev list and see what others think.


Reply to this email directly or view it on GitHub:
#460 (comment)

Copy link

@wuwenlun wuwenlun left a comment

Choose a reason for hiding this comment

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

461

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

4 participants