Remove checks for existing semicolons and newlines in concat_javascript_sources #310
Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| @@ -90,16 +90,18 @@ def string_end_with_semicolon?(str) | |||
| end | |||
|
|
|||
| # Internal: Accumulate asset source to buffer and append a trailing | |||
| # semicolon if necessary. | |||
| # semicolon and newline. This may result in duplicated semicolons and | |||
There was a problem hiding this comment.
We don't need to explain how it was before, so you can remove the section inside parentheses.
There was a problem hiding this comment.
I've removed it in a81958c
|
Thank you for the pull request. |
|
It seems that the CHANGELOG no longer follows the form mentioned in the contributing guidelines, so I wasn't sure if there's a new process. Let me know and I'll update the CHANGELOG however is recommended. |
|
Add the new changelog entry above the last release |
|
OK, I updated the changelog in 0d27cc5 |
|
Could you squash your commits? |
…pt_sources This may result in duplicated semicolons and newlines, but we don't mind because they'll be removed by the minifier, and these checks were very expensive.
rmacklin
force-pushed
the
remove-checks-for-extra-semicolons-and-newlines
branch
from
a81958c to
4715c16
Compare
|
I was just going to ask if you would like me to squash :) Done. |
|
Thank you so much for the pull request. #311 had a different approach that I preferred since this code was added to make source maps indexing working and removing it may break source maps. |
|
I just saw the discussion that has happened in #311 after it was merged. In particular the comments that:
drew my interest. So I was wondering: do you have any examples of broken source maps caused by this approach? I don't use source maps so it'd be nice to have something to start from. |
Closes #300
Unfortunately, given how widely used
concat_javascript_sourcesis, this required changing a lot of tests. It would be nice if we could remove some of the duplication in these tests (so that similar changes would not require updating this many tests), but that can come in another PR.