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

fix lint errors caused by bad-sim-text.js #736

Closed
35 tasks done
zepumph opened this issue Jan 17, 2019 · 15 comments
Closed
35 tasks done

fix lint errors caused by bad-sim-text.js #736

zepumph opened this issue Jan 17, 2019 · 15 comments

Comments

@zepumph
Copy link
Member

zepumph commented Jan 17, 2019

from #728, there were 74 errors that were caused by adding bad-sim-text lint rule:

Here are the affected repos:

To fix this, one of three actions should occur:

  1. Replace that usage with something else as the code review guidelines suggest.
  2. ignore this error by following the strategy that done in phetsims/wave-interference@e2a72ce. Make sure that usages are well documented!!!!
  3. Decide that the directory that the error occurs in doesn't belong being linted by sim specific lint rules, and add a .eslintrc.js file that specifies the correct lint rules, see chipper/eslint/README for more details.

To trigger there errors, got to chipper/eslint/sim_eslintrc.js and change the bad-sim-text rule to error (value of 2). Assigning all responsible devs for above sims. I assigned myself to common code repos because they may be less straightforward.

@zepumph
Copy link
Member Author

zepumph commented Jan 17, 2019

Actually regarding number "2" above, there are two ways to ignore the rule, and since the rule is set up to apply to the whole file (not individual lines), I think I prefer cd675bd

I think we should have a vote attached to this comment; to vote add a reaction to this comment.

If you would like to use eslint-disable at the top of the file (cd675bd) add a heart to this comment.

If you would like to use // eslint-disable-line at the first line of code ( phetsims/wave-interference@e2a72ce) add a hooray to this comment.

EDIT: developer discretion sounds totally find for this too.

pixelzoom added a commit to phetsims/acid-base-solutions that referenced this issue Jan 17, 2019
pixelzoom added a commit to phetsims/beers-law-lab that referenced this issue Jan 17, 2019
pixelzoom added a commit to phetsims/models-of-the-hydrogen-atom that referenced this issue Jan 17, 2019
pixelzoom added a commit to phetsims/molarity that referenced this issue Jan 17, 2019
pixelzoom added a commit to phetsims/reactants-products-and-leftovers that referenced this issue Jan 17, 2019
pixelzoom added a commit to phetsims/ph-scale that referenced this issue Jan 17, 2019
@pixelzoom
Copy link
Contributor

My sims are fixed, all passing lint.

@pixelzoom pixelzoom removed their assignment Jan 17, 2019
zepumph added a commit to phetsims/optics-lab that referenced this issue Jan 18, 2019
zepumph added a commit to phetsims/scenery that referenced this issue Jan 18, 2019
zepumph added a commit to phetsims/joist that referenced this issue Jan 18, 2019
zepumph added a commit to phetsims/phet-core that referenced this issue Jan 18, 2019
zepumph added a commit to phetsims/phetcommon that referenced this issue Jan 18, 2019
zepumph added a commit that referenced this issue Jan 18, 2019
zepumph added a commit to phetsims/scenery-phet that referenced this issue Jan 18, 2019
zepumph added a commit to phetsims/sun that referenced this issue Jan 18, 2019
@zepumph
Copy link
Member Author

zepumph commented Jan 18, 2019

The repos assigned to me are now completed.

@zepumph zepumph removed their assignment Jan 18, 2019
zepumph added a commit that referenced this issue Jan 18, 2019
pixelzoom added a commit that referenced this issue Jan 24, 2019
Signed-off-by: Chris Malley <[email protected]>
@jbphet
Copy link
Contributor

jbphet commented Jan 24, 2019

Works for me.

pixelzoom added a commit to phetsims/tambo that referenced this issue Jan 24, 2019
pixelzoom added a commit to phetsims/tambo that referenced this issue Jan 24, 2019
jbphet added a commit to phetsims/area-builder that referenced this issue Jan 30, 2019
jbphet added a commit to phetsims/balancing-act that referenced this issue Jan 30, 2019
jbphet added a commit to phetsims/faradays-law that referenced this issue Jan 30, 2019
jbphet added a commit to phetsims/gene-expression-essentials that referenced this issue Jan 30, 2019
jbphet added a commit to phetsims/states-of-matter that referenced this issue Jan 30, 2019
@jbphet
Copy link
Contributor

jbphet commented Jan 30, 2019

I've fixed the sims for which I am the responsible developer and have committed the change to the 'bad-sim-text' value to turn on the stricter checking. I'll watch CT for the next hour or so to make sure it doesn't cause a bunch of failures.

@jbphet
Copy link
Contributor

jbphet commented Jan 30, 2019

I'm not seeing any unusual failures on CT, so I think this can be closed. I'm going to assign it back to @zepumph since I think everything is done but would like someone who is more familiar with the issue to make sure I'm not missing anything.

@jbphet jbphet assigned zepumph and unassigned jbphet Jan 30, 2019
@samreid
Copy link
Member

samreid commented Jan 31, 2019

If you would like to use eslint-disable at the top of the file (cd675bd) add a heart to this comment.

If you would like to use // eslint-disable-line at the first line of code ( phetsims/wave-interference@e2a72ce) add a hooray to this comment.

bad-text should be rewritten so that it can provide line-specific failures, then we can use eslint-disable-line in the correct places.

@pixelzoom
Copy link
Contributor

Should this still be labeled for developer meeting? If so, what feedback do you want?

@zepumph
Copy link
Member Author

zepumph commented Jan 31, 2019

bad-text should be rewritten so that it can provide line-specific failures, then we can use eslint-disable-line in the correct places.

lets talk over in #738

that and #728 are follow up issues. Closing

@zepumph zepumph closed this as completed Jan 31, 2019
jessegreenberg added a commit to phetsims/greenhouse-effect that referenced this issue Apr 21, 2021
zepumph added a commit to phetsims/perennial that referenced this issue Oct 22, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Oct 22, 2024
zepumph pushed a commit to phetsims/perennial that referenced this issue Oct 22, 2024
zepumph pushed a commit to phetsims/perennial that referenced this issue Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants