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

feat: eslint #78

Merged
merged 10 commits into from
Nov 4, 2020
Merged

feat: eslint #78

merged 10 commits into from
Nov 4, 2020

Conversation

vltansky
Copy link
Member

@vltansky vltansky commented Nov 3, 2020

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

This PR adds eslint configs (extends eslint:recommended, airbnb/base, plugin:prettier/recommended) - run npm run lint, lint workflow for PRs and VScode settings that enables eslint and prettier for the project by default

@Jeroen-Matthijssens
Copy link

I was wondering how everyone else feels about a couple of things.

There is a file added here .vscode/settings.json. This is a configuration file for a specific editor. If I use a different editor that file is pretty much useless to me, but perhaps I am willing to add specific configuration for my editor of choice. However this may lead to an arbitrary large collection of configuration for random editors that happend to have been used on the project at some point in history. To me that does not feel very nice. On the flip side if you leave out, you may miss useful configuration or need some other mechanism for distributing it.

So I was wondering how other people approach this.

A second thing I noticed is that there appears to be a merge commit from master into this branch. Personally I would have rebased this branch on top of master, perhaps squashing some commits. But there may be reasons I am missing which would make that a bad idea.

@coliff
Copy link
Member

coliff commented Nov 4, 2020

Adding eslint workflow for this project is a great idea. 👍 I'm not in favour of adding a .vscode/ directory though. VS Code is awesome, it's my code editor of choice and is the most popular editor, but I think this project should be platform/editor-agnostic and would suggest to not commit the VS Code settings.

@vltansky
Copy link
Member Author

vltansky commented Nov 4, 2020

.vscode settings giving a power of eslint and prettier on-save instead of only pre commit. I think this is awesome functionality in a price of one json file.. I see a lot of projects leveraging this

@coliff
Copy link
Member

coliff commented Nov 4, 2020

ok, fair points. :-) If we do add .vscode/settings.json then I think it'd make sense to add an .vscode/extensions.json file with this so it recommends the extensions. I use a similar thing with projects I work on:

{
  "recommendations": [
    "EditorConfig.EditorConfig",
    "esbenp.prettier-vscode"
  ]
}

@vltansky
Copy link
Member Author

vltansky commented Nov 4, 2020

ok, fair points. :-) If we do add .vscode/settings.json then I think it'd make sense to add an .vscode/extensions.json file with this so it recommends the extensions. I use a similar thing with projects I work on:

{
  "recommendations": [
    "EditorConfig.EditorConfig",
    "esbenp.prettier-vscode"
  ]
}

Done

@roblarsen
Copy link
Member

@Jeroen-Matthijssens I will squash and merge this PR into master, so the commit into the main branch will be clean.

I'm +1 on the VS Code stuff. It's relatively unobtrusive and I wouldn't propose adding anything for any other editors since I too use it exclusively.

We can always change our minds.

@vltansky can you fix the conflicts? I'll merge.

ecmaVersion: 12,
},
rules: {
"no-console": "off",
Copy link
Member

Choose a reason for hiding this comment

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

pretty big one for this project

const packageName = "html5-boilerplate";
const tempDir = os.tmpdir() + `/${packageName}-staging`;
const tempDir = `${os.tmpdir()}/${packageName}-staging`;
Copy link
Member

Choose a reason for hiding this comment

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

👍

@vltansky
Copy link
Member Author

vltansky commented Nov 4, 2020

@roblarsen fixed

@roblarsen roblarsen merged commit e7a4db0 into h5bp:master Nov 4, 2020
@roblarsen
Copy link
Member

Thanks!

@github-actions
Copy link

github-actions bot commented Nov 4, 2020

🎉 This PR is included in version 0.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@roblarsen
Copy link
Member

roblarsen commented Nov 4, 2020

I forgot semantic-release. That's awesome- although since I forgot, it's not what I want from a communication perspective. That's not a feature that an end-user cares about at all so releasing a new version is silly.

@vltansky
Copy link
Member Author

vltansky commented Nov 4, 2020

v0.3 released as this PR contained feat: prefix in one of the commits
https://github.com/semantic-release/semantic-release#commit-message-format
Although nice to see that semantic-release works and how it works, this kind of PR shouldn't trigger release (as it doesn't have any feature, only development tools). So need to be careful with prefixes (telling this to myself)

P.S
Didn't see your comment while writing this. Agree, that's my mistake here.

@roblarsen
Copy link
Member

Yeah, it's so cool to see it work. I just forgot about it when I bundled up your commit messages. We can manage it better in the future. No worries at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants