Discussion: how to integrate with awesome-lists

[transitive-bullshit]

I'd like to start a discussion around the status of awesome-lint and how it should relate to the awesome-list project going forwards.

A few questions we should aim to gather consensus around:

1. What should the expected acceptance criteria be for awesome-lint in order to consider enforcing linting as a necessary constraint for future awesome-list PRs?
2. Would it make sense to gradually lint existing lists in order to maintain a high quality? One thing I've thought about is that we could automate reaching out to existing list maintainers with the request that any lists that fail linting would be removed in 60 days unless they're updated accordingly.

[itaisteinherz]

1. What should the expected acceptance criteria be for awesome-lint in order to consider enforcing linting as a necessary constraint for future awesome-list PRs?

Probably that it lints every requirement (or most of them, in case some are complex and hard to check) in the Awesome list requirements.

2. Would it make sense to gradually lint existing lists in order to maintain a high quality? One thing I've thought about is that we could automate reaching out to existing list maintainers with the request that any lists that fail linting would be removed in 60 days unless they're updated accordingly.

We could do so, but I think the most efficient way (at the moment) to reduce the load on Sindre and test out awesome-lint is to automatically lint every open pull request. Then, the bot would comment whether or not linting passed, and if not the errors. If, in the process, the bot also labels each PR as either passing linting or not, that would probably make it easier to find and review PRs which are ready to be merged. The bot should also request changes in PRs where there are linting errors.

@transitive-bullshit @sindresorhus Any thoughts?

[sindresorhus]

We could do so, but I think the most efficient way (at the moment) to reduce the load on Sindre and test out awesome-lint is to automatically lint every open pull request. Then, the bot would comment whether or not linting passed, and if not the errors. If, in the process, the bot also labels each PR as either passing linting or not, that would probably make it easier to find and review PRs which are ready to be merged. The bot should also request changes in PRs where there are linting errors.

👍

[transitive-bullshit]

The immediate goal as far as I can tell would then be to integrate awesome-lint into PRs on the awesome repo, as well as updating the awesome list requirements to note that we expect new lists to pass awesome-lint CI.

Some notes / concerns for adding awesome-lint to CI:

1. If we include a package.json and use awesome-lint in CI from the awesome repo, it'll change that repo's GitHub identity from "unknown language" to "JavaScript". This may be just fine, but I wanted to verify this with @sindresorhus first.
2. In the PR CI, we'll need a way of extracting only the added lines in the PR commit(s) and extract any awesome list repo links from them, and then run awesome-lint on those links. I'm sure this is possible within travis-ci, I just haven't done it before.
3. An alternative to #2 would be that our CI job extracts all repo links from the master awesome list and just whitelists as many existing lists as possible such that we would only run awesome-lint on new lists going forwards.

As long as Sindre is okay with #1, I'm leaning towards #3 for implementing linting in the main awesome list.

Thoughts / awesome jokes? 👍

[sindresorhus]

If we include a package.json and use awesome-lint in CI from the awesome repo, it'll change that repo's GitHub identity from "unknown language" to "JavaScript". This may be just fine, but I wanted to verify this with @sindresorhus first.

👍 There are ways to override that if it happens: https://github.com/github/linguist#overrides

As long as Sindre is okay with #1, I'm leaning towards #3 for implementing linting in the main awesome list.

I'm good with 3.

[itaisteinherz]

Both #2 and #3 seem fine to me, but there are a few things I'd like to point out.

First of all, #2 seems like a much more efficient method than #3, since the CI job only needs to process the changes introduced by the current commit / PR commits, instead of loading the master awesome list, comparing it and the repo at its current state and then processing the differences. This could make the CI job significantly slower (but please correct me if I'm wrong, as I've never done this before).

An alternative to #2 would be that our CI job extracts all repo links from the master awesome list and just whitelists as many existing lists as possible such that we would only run awesome-lint on new lists going forwards.

An issue I see with #3 is that some of the lists which have already been added to the master awesome list wouldn't pass linting (e.g. awesome-vagrant, awesome-machine-learning and awesome-rails), but these lists would never get linted if #3 is implemented.

Initially, I thought the best way to integrate awesome-lint with the master awesome list was to create a GitHub bot which would handle everything, but after additional thinking and @transitive-bullshit's suggestions, it seems like a better solution would be to lint a commit / PR in a CI job using either #2 or #3, and then perform further actions (e.g. creating automated review comments based on the linting results) using a GitHub bot.
Implementing linting in the main awesome list using a CI job should (hopefully) be good for now, and we could always create a bot down the road.

[transitive-bullshit]

@itaisteinherz I think there are a few misunderstandings, so let me try to clear them up :)

First of all, #2 seems like a much more efficient method than #3

No, they would be 99% the same efficiency-wise. There'd be no "calculating the diff" with #3; e.g., you just parse all the lists, remove lists which are in the whitelist, and lint any remaining lists. There is very little overhead here, and it'll be significantly easier to implement than #2.

An issue I see with #3 is that some of the lists which have already been added to the master awesome list wouldn't pass linting, but these lists would never get linted if #3 is implemented.

The whole point of the whitelist is that you'd start with all existing lists in it (because we don't care about linting them at the moment) and remove passing lists over time. Eventually, the goal would be to get to a state where all lists are passing, but we need to do things incrementally here, and the whitelist provides a great pattern for doing that.

Implementing linting in the main awesome list using a CI job should (hopefully) be good for now, and we could always create a bot down the road.

Agreed; CI pass / fail as the primary signal for now, in addition to some notes in the awesome PR requirements doc. We can get fancier with a bot down the road if things go well 👍

[itaisteinherz]

Thanks for the clarifications 🙂 Now that I have a clear idea of what you suggested in #3, it does look like the better option.

If we include a package.json and use awesome-lint in CI from the awesome repo

Just to make sure we're on the same page: what you meant is that we'll add an option to awesome-lint to lint a main list (i.e. lists of awesome lists) instead of an awesome list, right?
If so, we don't need to add a package.json to the main awesome list - assuming the option is named --main, we can just run npx awesome-lint --main in CI 😎

[transitive-bullshit]

what you meant is that we'll add an option to awesome-lint to lint a main list (i.e. lists of awesome lists) instead of an awesome list, right?

That's definitely a reasonable option that I honestly hadn't thought of. I just submitted an initial PR sindresorhus/awesome#1394 that adds the test functionality over there, but I'm very open to switching things around depending on how the discussion goes :)

[transitive-bullshit]

@sindresorhus friendly ping 😄 😄 😄

[sindresorhus]

If so, we don't need to add a package.json to the main awesome list - assuming the option is named --main, we can just run npx awesome-lint --main in CI 😎

We still need the whitelist and stuff. I think it's easier to just have it as a Node.js script for now so we can easily tweak things. I totally agree it would be nice to make it just a flag of the linter sometime in the future though.