Skip to content

Replace ^M with new line (\r) in mustache template#3865

Merged
wing328 merged 7 commits intoswagger-api:masterfrom
wing328:fix_line_break
Sep 26, 2016
Merged

Replace ^M with new line (\r) in mustache template#3865
wing328 merged 7 commits intoswagger-api:masterfrom
wing328:fix_line_break

Conversation

@wing328
Copy link
Copy Markdown
Contributor

@wing328 wing328 commented Sep 24, 2016

PR checklist

  • Read the contribution guildelines.
  • Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Replace ^M with new line (\r) in the mustache templates under modules/swagger-codegen/src/main/resources

TODO: update CI (travis) to catch the issue moving forward.

@wing328 wing328 merged commit fa12cd3 into swagger-api:master Sep 26, 2016
@wing328 wing328 deleted the fix_line_break branch September 26, 2016 08:50
Copy link
Copy Markdown
Contributor

@ePaul ePaul left a comment

Choose a reason for hiding this comment

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

I don't see any change in the model.mustache file, so maybe that was added in a different pull request.

/**
* Pet
* A pet for sale in the pet store
*/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, now the comment is here twice?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to be happening for all the java model files.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't see duplicated comments. Can you share a URL to the lines of code with duplicated comments in the latest master?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

diff, raw file. It looks like this:

 /**
  * A pet for sale in the pet store
  **/

  /**
   * A pet for sale in the pet store
   */

(This new review mode doesn't show enough context, nor does it easily link back to the file.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ePaul thanks for the details. I'll take a deeper look tomorrow.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ePaul fixed via #3887

@ePaul
Copy link
Copy Markdown
Contributor

ePaul commented Sep 27, 2016

@wing328 Just a note about terminology: \r and ^M represent the same thing, the carriage return character (aka CR, 0x0D, U+000D). Newline is also known as Line feed, 0x0A, LF, \n, ^J, U+000A.

I think you meant to say you replaced all CRLF sequences (and possibly some single CRs?) by just LF (\n). (I couldn't really see what changed in the diffs, but that would be my guess as this fixes what Windows editors do wrong sometimes.)

@wing328
Copy link
Copy Markdown
Contributor Author

wing328 commented Sep 27, 2016

@Epual you're definitely right. Actually I used \r when using grep and ^M in vim. The title was (incorrectly) "borrowed" from a stackoverflow question and I'll be more careful with the terminology moving forward.

acramatte added a commit to comerge/swagger-codegen that referenced this pull request Oct 4, 2016
* upstream/master: (79 commits)
  add undertow
  Add a new cli command to output version information (2nd attempt) swagger-api#3892 (swagger-api#3899)
  fix python flask controller without tag (default_controller)
  [aspnet5] Fix basePath application to operations (swagger-api#3911)
  Bugfix/issue 3723 (swagger-api#3726)
  Cgardens nested object regex (swagger-api#3879)
  [Cpprest] Fixing issue swagger-api#3773 (swagger-api#3876)
  escape callback parameter for java(okhttp) and python
  fix warning in html generator
  [PHP] fix PHPUnit invocation, add basic phpunit.xml.dist (swagger-api#3864)
  [Java] Remove duplicated model description in Spring, JAX-RS models (swagger-api#3887)
  [PHP] Better PSR2 compatibility (swagger-api#3863)
  Mention security script in pull request template
  [Swift] Use thread safe manager dictionary
  Replace ^M with new line (\r) in mustache template (swagger-api#3865)
  [swfit] fix url param with base name
  [JaxRS]Show correct default value on CLI option description (swagger-api#3862)
  add title, description to HTML output (swagger-api#3860)
  fix trailing comma in go api client
  fix typescript-fetch base path by removing ending slash
  ...
@wing328
Copy link
Copy Markdown
Contributor Author

wing328 commented Jan 9, 2017

FYI. Added scripts to detect the issue moving forward via #4526

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.

2 participants