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

Refactor and update index.js to use fdir() #5653

Merged
merged 35 commits into from
May 13, 2022

Conversation

queengooborg
Copy link
Collaborator

@queengooborg queengooborg commented Feb 5, 2020

This PR updates index.js in a couple of ways:

  1. Replaces the use of the arguments keyword with a defined ...files argument (for both consistency and function legibility)
  2. Adds a comment to indicate this file is a part of BCD
  3. Updates the JSDoc comments
  4. Utilizes fdir() to walk the file tree

@queengooborg queengooborg added the infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project label Feb 5, 2020
@queengooborg queengooborg changed the title Fix ESLint errors + JSDoc comments for index.js Refactor and update index.js Feb 7, 2020
@ddbeck
Copy link
Collaborator

ddbeck commented Feb 17, 2020

A couple questions on this one

1. Turns the `load()` function into one using arrow syntax

Why?

3. Adds the CC0 copyright notice

Do you know where this text originated? I've started to see it on some scripts, but I have no idea where this text comes from originally. It's… off in some ways and I'm not sure whether we should be reproducing it.

@ExE-Boss
Copy link
Contributor

3. Adds the CC0 copyright notice

Do you know where this text originated? I've started to see it on some scripts, but I have no idea where this text comes from originally. It's… off in some ways and I'm not sure whether we should be reproducing it.

It matches what’s in: https://www.mozilla.org/MPL/headers/#pd-c.

There’s also:

CC0 1.0 Universal

@queengooborg
Copy link
Collaborator Author

queengooborg commented Feb 17, 2020

For the arrow function syntax, we’ve got a lot of functions in our code that use the arrow function syntax. I figured it makes sense to maintain function style consistency! The primary part is the inclusion of the “files” argument, however — when I first saw the function, I was quite confused about how it functioned since I didn’t see the code down below that uses the “arguments” keyword. None of the other loading functions throughout BCD use it; they all have the same “files” argument.

As for the copyright notice, this is because BCD as a whole is licensed under the CC0 license!

@queengooborg queengooborg mentioned this pull request Mar 27, 2020
23 tasks
@ddbeck ddbeck self-assigned this Oct 27, 2020
@ddbeck ddbeck removed their request for review December 17, 2020 16:22
@Elchi3 Elchi3 removed their request for review February 18, 2021 17:16
Base automatically changed from master to main March 24, 2021 12:54
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@github-actions github-actions bot added the dependencies ⛓️ Pull requests that update a dependency package or file. label May 12, 2022
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, but unless I'm mistaken the main change of this PR is to migrate to fdir() (from fs.readdirSync()), and this should be mirrored at least in the PR title (which is used for the squash merge commit), but ideally also in the PR description.

@github-actions
Copy link

This pull request has merge conflicts that must be resolved before we can merge this.

@queengooborg queengooborg changed the title Refactor and update index.js Refactor and update index.js to use fdir() May 13, 2022
@queengooborg queengooborg enabled auto-merge (squash) May 13, 2022 20:26
@queengooborg queengooborg merged commit 8ad738a into mdn:main May 13, 2022
@queengooborg queengooborg deleted the eslint/index branch May 13, 2022 20:26
@queengooborg
Copy link
Collaborator Author

Uh oh... @foolip this apparently broke the collector because fdir is not crawling relative to the root of index.js. Sending in a fix now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies ⛓️ Pull requests that update a dependency package or file. infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants