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

[update] Adds DSC style linting #108

Merged
merged 2 commits into from
Apr 3, 2018
Merged

Conversation

yowainwright
Copy link
Contributor

@yowainwright yowainwright commented Apr 2, 2018

DSC style—eslint

  • adds eslint
  • add eslint-config-dollarshaveclub
  • remove lots of ignored files
  • update linting in the few files that needed it (mainly simicolons)

Heyo, let me know what I can do to provide 👌PRs. ~danka

I can help with some config stuff to start. Then PRs and publishing—if needed.

@yowainwright
Copy link
Contributor Author

yowainwright commented Apr 2, 2018

⬇️Weird...tests passed locally. 🤔

Part of the config updates I am thinking about is to:

  • switch to CircleCi,
  • re-enable Greenkeeper, and
  • add auto merging for approved and passing PRs.

@@ -1,48 +1,8 @@
module.exports = {
root: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I highly recommend against this. These are the Ember defaults, and most apps and addons adhere to them. If there are specific rules we need to add on top, we could do so or maybe even use the DSC plugin alongside these.

Copy link
Contributor Author

@yowainwright yowainwright Apr 2, 2018

Choose a reason for hiding this comment

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

@rwwagner90 I will follow up with my team and add that to eslint-config-dollarshaveclub if it makes sense for DSC—per your recommendation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it makes sense for DSC, but it does make sense for this and your other popular, open source, Ember addons. They are used heavily by the community and have several active contributors. I think it would be preferential to keep the linting the same it has been, in line with the Ember community, here.

package.json Outdated
"description": "Scroll to top with preserved browser history scroll position",
"directories": {
"doc": "doc",
"test": "tests"
},
"scripts": {
"build": "ember build",
"lint:js": "eslint ./*.js addon addon-test-support app config lib server test-support tests",
"eslint": "eslint . --fix",
"lint:ci": "eslint .",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with these commands. lint:js is added by ember-cli and will be expected to exist by most users. We should strive to keep things as close to the recommended path as possible, to make it easiest for contributors.

@yowainwright
Copy link
Contributor Author

@rwwagner90 @snewcomer How do you feel about the other bullets?

Tests take a really long time. Anything we can do there?

@RobbieTheWagner
Copy link
Contributor

@yowainwright I generally use Travis, since it is the default out of the box. Is there a benefit to CircleCI? Greenkeeper is fine with me, though it has been less beneficial to me, as of late. Auto merging with an approval could be nice, but not sure how much work it is to set up.

Tests take a long time because ember-try runs them against a bunch of Ember versions, so we can ensure compatibility.

@yowainwright
Copy link
Contributor Author

yowainwright commented Apr 2, 2018

  1. CircleCi supports cron within it's yml config which Travis does not. CircleCi can/will run cron jobs with more frequency than what Travis supports. This feature allows us to do things like have bots automerge approved PRs—which is awesome since tests take so long.
  2. DSC uses CircleCi.

There are more reasons but point 1 is a huge win. Point 2 can help too—there can be a lot more support from the DSC team.

@RobbieTheWagner
Copy link
Contributor

@yowainwright while I appreciate the desire to make things align with DSC, since you own the repo, there has been a lot of community traction on this and several other of your addons. I suppose my argument here is "if it ain't broke, don't fix it". We've been chugging along on features and improvements and could really use more involvement from DSC on helping review and shape those things, and I wouldn't shake up all the code standards and CI requirements on everyone as the first step, but again it is your addon and I am happy to help with whatever direction you decide, just wanted to make my thoughts known 😃 .

@yowainwright
Copy link
Contributor Author

@rwwagner90 thanks for your candidness.

@yowainwright yowainwright force-pushed the add-dsc-style-linting branch from fb0846c to bf9f4a2 Compare April 3, 2018 03:12
@yowainwright yowainwright merged commit dc98e47 into master Apr 3, 2018
@yowainwright yowainwright deleted the add-dsc-style-linting branch April 3, 2018 03:50
@snewcomer
Copy link
Collaborator

I'm afraid this PR is going to make it much harder for new ppl from the community to contribute by enforcing these linting standards. Moreover, every attempt to upgrade this will lead to large suggested diffs, making it much harder to pick out the recommended changes from the blueprints.

I think what we really needed before any work was done was a review of outstanding PR's. Specifically #101.

@RobbieTheWagner
Copy link
Contributor

I totally agree with this ☝️ . I would have loved to keep iterating on the wonderful contributions from people like @snewcomer, rather than spending a bunch of time trying to learn all these new requirements that do not mesh with the Ember standards people are used to.

@yowainwright
Copy link
Contributor Author

yowainwright commented Apr 3, 2018

@rwwagner90 @snewcomer

There is a lot of push back! 😔

I've never been asked to help with a repository and then received such feedback. I sensed a lot of frustration yesterday so I reached out to other teammates to decide what I should do. I'm going to consult with the team about this feedback—and friends that started this addon. I am open to reverting this PR and continuing to help with a hands-off approach—or not help. I'm not trying to affect others fun/work.

My approach over the last 2 days was to add the featuring in CI so that the team could easily support the plugin to make sure that it stays up-to-date—which is how I perceived email I received. These updates were to involve adding team-like linting, and then adding team-like CI. I was then go in an start working on issues and adding more tests.

@snewcomer
Copy link
Collaborator

snewcomer commented Apr 3, 2018

@yowainwright Well, we are all in this together so can work with whatever your team decides 👍 . I think where I may have had a slightly wrong assumption is the "community" aspect of this addon. It definitely can live on a sliding scale where an open piece of software can be specific for an organization and maintained by it's members, and on the other end, purely a community maintained solution.

What we were originally hoping for as a first step was to give a responsible, active maintainer that is familiar with the codebase (i.e. Robert W) npm publish rights.

I think your work is great! Just probably need to clarify in my own head how this addon relates to the community as a whole.

@yowainwright yowainwright mentioned this pull request Apr 9, 2018
5 tasks
@RobbieTheWagner
Copy link
Contributor

@yowainwright I don't seem to be able to revert this via GitHub. Can you revert this please?

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

Successfully merging this pull request may close these issues.

4 participants