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

Never print comments in compact mode #3146

Merged
merged 2 commits into from
Dec 7, 2015
Merged

Never print comments in compact mode #3146

merged 2 commits into from
Dec 7, 2015

Conversation

amasad
Copy link
Member

@amasad amasad commented Dec 7, 2015

No description provided.

@codecov-io
Copy link

Current coverage is 84.91%

Merging #3146 into master will not affect coverage as of 7580de8

@@            master   #3146   diff @@
======================================
  Files          214     214       
  Stmts        15589   15589       
  Branches      3326    3326       
  Methods          0       0       
======================================
  Hit          13237   13237       
  Partial        680     680       
  Missed        1672    1672       

Review entire Coverage Diff as of 7580de8

Powered by Codecov. Updated on successful CI builds.

@@ -231,7 +231,7 @@ export default class Printer extends Buffer {
if (this.format.shouldPrintComment) {
return this.format.shouldPrintComment(comment.value);
} else {
if (comment.value.indexOf("@license") >= 0 || comment.value.indexOf("@preserve") >= 0) {
if (!this.format.compact && comment.value.indexOf("@license") >= 0 || comment.value.indexOf("@preserve") >= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be !compact && (@license || @preserve)?

Copy link
Member Author

Choose a reason for hiding this comment

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

The precedence works out. || has higher precedence

On Monday, December 7, 2015, René Kooi notifications@github.com wrote:

In packages/babel-generator/src/printer.js
#3146 (comment):

@@ -231,7 +231,7 @@ export default class Printer extends Buffer {
if (this.format.shouldPrintComment) {
return this.format.shouldPrintComment(comment.value);
} else {

  •  if (comment.value.indexOf("@license") >= 0 || comment.value.indexOf("@preserve") >= 0) {
    
  •  if (!this.format.compact && comment.value.indexOf("@license") >= 0 || comment.value.indexOf("@preserve") >= 0) {
    

should this be !compact && (@license || @preserve)?


Reply to this email directly or view it on GitHub
https://github.com/babel/babel/pull/3146/files#r46809012.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't && have higher precedence?

Copy link
Member Author

Choose a reason for hiding this comment

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

huh, maybe, I'll just add parens ;P

Copy link
Member

Choose a reason for hiding this comment

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

Ex:

false && true  || true      // returns true
false && (true || true)     // returns false

amasad added a commit that referenced this pull request Dec 7, 2015
Never print comments in compact mode
@amasad amasad merged commit aaf8348 into master Dec 7, 2015
@amasad amasad deleted the no-comment branch December 7, 2015 18:38
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 8, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants