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: pass cwd to formatters (refs eslint/rfcs#57) #13392

Merged
merged 8 commits into from Nov 30, 2021
Merged

Conversation

mysticatea
Copy link
Member

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[X] Add something to the core: eslint/rfcs#57

What changes did you make? (Give an overview)

This is the implementation of eslint/rfcs#57. It updates the adapter of ESLint.prototype.loadFormatter method to pass cwd to formatters. The cwd is useful to make relative paths from absolute paths for readability.

Is there anything you'd like reviewers to focus on?

Is this direction right?

@mysticatea mysticatea added enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jun 5, 2020
@mad-briller
Copy link

any news on this? would like to write a custom formatter that outputs relative paths for ci/cd purposes and this is a requirement to do that and it's over a year old

another issue was raised in the past about this:
#13376

@nzakas nzakas marked this pull request as ready for review October 9, 2021 00:40
@nzakas
Copy link
Member

nzakas commented Oct 9, 2021

Looks like we lost track of this and there are now conflicts that need to be addressed. We will take another look after v8.0.0.

@nzakas nzakas added this to Ready for Dev Team in Triage Oct 9, 2021
@mdjermanovic mdjermanovic changed the title Update: pass cwd to formatters (refs eslint/rfcs#57) feat: pass cwd to formatters (refs eslint/rfcs#57) Oct 26, 2021
@btmills btmills added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Nov 4, 2021
@btmills
Copy link
Member

btmills commented Nov 4, 2021

This implements an approved RFC, so re-labeling as accepted. We just need someone to resolve the merge conflicts so this is reviewable. I'm happy to pick it up unless someone else gets to it first.

@linux-foundation-easycla

This comment has been minimized.

@btmills
Copy link
Member

btmills commented Nov 21, 2021

This is ready for review.

The rebase turned out to be a little more complicated than expected because this was previously using the in-memory filesystem that we've since stopped using. I created a fixture instead. There were a few tests that use the formatter fixtures directory as a target, and I had to update those with the new file.

mysticatea's commits are covered by the old CLA, so I've hidden the comment from the CLA bot.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for finishing this up.

@mdjermanovic mdjermanovic moved this from Ready for Dev Team to Pull Request Opened in Triage Nov 24, 2021
@mdjermanovic
Copy link
Member

mdjermanovic commented Nov 24, 2021

@btmills merging #15243 caused conflicts in the tests that lint the content of fixtures/formatters/. Can you resolve the conflicts? Once we merge this, I'll make a PR to change those tests to use some other files, so that this doesn't happen again.

btmills and others added 4 commits November 26, 2021 19:18
I need to add another formatter to this directory that will shift all of
these results, so I'm making these assertions more explicit so it's
obvious why a seemingly-unrelated change will cause these to fail.
@btmills
Copy link
Member

btmills commented Nov 27, 2021

Merge conflicts resolved!

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

Just a couple of suggestions about the docs.

docs/developer-guide/working-with-custom-formatters.md Outdated Show resolved Hide resolved
docs/developer-guide/working-with-custom-formatters.md Outdated Show resolved Hide resolved
docs/developer-guide/working-with-custom-formatters.md Outdated Show resolved Hide resolved
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
btmills and others added 3 commits November 30, 2021 01:23
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@btmills
Copy link
Member

btmills commented Nov 30, 2021

Thanks for those improvements. I committed those and fixed a merge conflict caused by the markdownlint update that was merged a few hours ago.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 808ad35 into main Nov 30, 2021
@mdjermanovic mdjermanovic deleted the cwd-in-formatters branch November 30, 2021 12:08
Triage automation moved this from Pull Request Opened to Complete Nov 30, 2021
fasttime added a commit to origin-1/gulp-eslint-new that referenced this pull request Dec 6, 2021
This replicates a new feature in ESLint 8.4:
eslint/eslint#13392
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators May 30, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label May 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
Archived in project
Triage
Complete
Development

Successfully merging this pull request may close these issues.

None yet

5 participants