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.

Sign up for GitHub

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

Jump to bottom

fix: adjust formatting in locales.hbs so it matches ESLint rules #466

Merged
merged 2 commits into from
Oct 7, 2020
Merged

fix: adjust formatting in locales.hbs so it matches ESLint rules #466

merged 2 commits into from
Oct 7, 2020

Conversation

HendrikThePendric
Copy link
Contributor

This is primarily just a quick fix to prevent the yarn lint command from listing these formatting errors in app-platform apps.

@HendrikThePendric HendrikThePendric self-assigned this Oct 6, 2020
@amcgee
Copy link
Member

amcgee commented Oct 6, 2020

Nice, thanks @HendrikThePendric !

I think this is fine and a good plan, however platform apps shouldn't break on files in src/locales since it's in .gitignore... at least I haven't seen it getting triggered except when that file is incorrectly in the git index... which app is triggering this eslint failure?

@HendrikThePendric
Copy link
Contributor Author

however platform apps shouldn't break on files in src/locales

Just to be clear: no nothing breaks, you're completely right. I'll explain what triggered me to create this PR:
When porting apps to the platform there is a step where you apply the new formatting (ESLint & Prettier) rules and inevitably end up fixing lots of ESLint errors. When working on those I find myself running yarn lint quite often, and it'd be nice if the list of js-errors wasn't cluttered by the errors in src/locales/index.js.

@HendrikThePendric HendrikThePendric mentioned this pull request Oct 7, 2020
Closed
@amcgee
Copy link
Member

amcgee commented Oct 7, 2020

Right, but I think lint should ignore the files in src/locales...?

@HendrikThePendric
Copy link
Contributor Author

Yeah good point.... I hadn't really seen this happen in other apps I ported to the app-platform. I've just verified two things:
A. src/locales is present in the .gitignore, see here
B. When I run yarn lint I do see these lint errors coming from src/locales/index.js

So not sure what's going there... However, regardless of all of this it would still be nice if the formatted file is compliant with our current set of lint rules.

@amcgee
Copy link
Member

amcgee commented Oct 7, 2020

@HendrikThePendric looks like that app has a custom .eslintignore - maybe try removing it

@amcgee
Copy link
Member

amcgee commented Oct 7, 2020

See cli-style docs on ignore files

amcgee
amcgee approved these changes Oct 7, 2020
View reviewed changes
Copy link
Member

@amcgee amcgee left a comment

Choose a reason for hiding this comment

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

Change LGTM!

@amcgee amcgee merged commit 658fa0b into master Oct 7, 2020
@amcgee amcgee deleted the fix-locales-formatting branch October 7, 2020 09:54
@HendrikThePendric
Copy link
Contributor Author

@HendrikThePendric looks like that app has a custom .eslintignore - maybe try removing it

Yes that was it 🎉 Thanks for that.

dhis2-bot added a commit that referenced this pull request Oct 7, 2020
@dhis2-bot
chore(release): cut 5.2.2 [skip ci]
93c41a6 
## [5.2.2](v5.2.1...v5.2.2) (2020-10-07)

### Bug Fixes

* adjust formatting in locales.hbs so it matches ESLint rules ([#466](#466)) ([658fa0b](658fa0b))
* encode username and password when posting to server in AuthBoundary ([#467](#467)) ([b0cc51d](b0cc51d))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 5.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amcgee amcgee amcgee approved these changes

Assignees

@HendrikThePendric HendrikThePendric

Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HendrikThePendric @amcgee @dhis2-bot