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

chore: license headers #123

Merged
merged 2 commits into from
Apr 9, 2024
Merged

chore: license headers #123

merged 2 commits into from
Apr 9, 2024

Conversation

trentm
Copy link
Member

@trentm trentm commented Apr 9, 2024

Refs: #45


Note that for this change I excluded .eslintrc.js from being formatted so that the diff from the .eslintrc.json was more obvious. It can be removed from the exclude patterns for a separate PR.

@trentm trentm requested a review from david-luna April 9, 2024 03:17
@trentm trentm self-assigned this Apr 9, 2024
@trentm
Copy link
Member Author

trentm commented Apr 9, 2024

The reason I changed from .eslintrc.json to .eslintrc.js is that I need to customize the config in each of the package dirs. The problem with eslint usage in the package dirs (e.g. in packages/mockotlpserver) is that it (a) runs with cwd set to the package dir, and (b) tries to load the "./scripts/license-header.js" from that dir. That file load fails. To work around that, I override that single rule in the sub dirs.

On the way to this workaround I tried:

  • eslint-plugin-header (being used by OTel JS), and
  • eslint-plugin-headers.

I rule out using those because they are awkward and most importantly, possibly destructive:
The former is just buggy in the face of top comments that it doesn't recognize (e.g. line-style comments, // this is my comment) or shebang lnes.

The later blindly replaces the top-comment in the file. So if you have a new file like this in VSCode, which is likely setup to run eslint --fix automatically on save:

// Here is some comment I spent some time on.

function hereIsSomeCode() {
  // ...
}

function thatIHaveBeenWorkingOn() {
}

then on save your carefully written top comment is blown away. You might not notice for a long while. If this is a new file then you likely don't have a git history to fall back on.

@trentm trentm merged commit c89c575 into main Apr 9, 2024
10 of 11 checks passed
@trentm trentm deleted the trentm/license-headers branch April 9, 2024 15:18
trentm added a commit that referenced this pull request Apr 9, 2024
I broke this with a bad merge on the #123 PR before merge it to main.
@trentm trentm mentioned this pull request Apr 9, 2024
trentm added a commit that referenced this pull request Apr 9, 2024
I broke this with a bad merge on the #123 PR before merge it to main.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants