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.

Sign up for GitHub

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

Jump to bottom

Moves @microsoft/eslint-formatter from sarif-sdk to sarif-js-sdk #23

Merged
merged 2 commits into from
May 8, 2021
Merged

Moves @microsoft/eslint-formatter from sarif-sdk to sarif-js-sdk #23

merged 2 commits into from
May 8, 2021

Conversation

scalvert
Copy link
Collaborator

@scalvert scalvert commented May 7, 2021 •
edited
Loading

Since the sarif-js-sdk didn't exist when the @microsoft/eslint-formatter-sarif package was first authored, it made sense to house it under the sarif-sdk. Now that this repository exists, that package is better aligned with this repository. It also gives us:

  • Prettier
  • eslint
  • jest for tests
  • the ability to leverage its sister package, @microsoft/jest-sarif to validate the produced SARIF output.

I attempted to keep the changes as small as possible, but the conflict that resulted from using mocha/chai with our eslint configuration made it more of a challenge to just merge in as is. Converting it to jest seemed prudent, though results in more files changed.

cc/ @eddynaka as per our discussion offline.

When Merging

Ensure correct attribution from source repositor ( using git log --pretty=format:"%an <%ae>" --no-merges main src/ESLint.Formatter/ | uniq -u in sarif-sdk):

Co-authored-by: Chris Raynor <[email protected]>
Co-authored-by: Chris Meyer <[email protected]>
Co-authored-by: Eddy Nakamura <[email protected]>
Co-authored-by: Larry Golding <[email protected]>
Co-authored-by: lukadlet <[email protected]>
Co-authored-by: Mike Huguet <[email protected]>
Co-authored-by: Rusty Scrivens <[email protected]>
Co-authored-by: tosmolka <[email protected]>

@scalvert scalvert added the enhancement New feature or request label May 7, 2021
scalvert
scalvert commented May 7, 2021
View reviewed changes
packages/eslint-formatter-sarif/__tests__/sarif-test.js

expect(log.runs[0].tool.driver.name).toBe('ESLint');
expect(log.runs[0].tool.driver.informationUri).toBe('https://eslint.org');
expect(log.runs[0].tool.driver.version).toMatch(semverRegex());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This assertion changed from the original implementation, since these tests executed a node that was able to resolve to a valid eslint package version. As such, it makes more sense to change the test, since this more closely matches reality anyway (a formatter would always expect to have an eslint that it's being run from)

@scalvert scalvert mentioned this pull request May 7, 2021
Open
@scalvert
Copy link
Collaborator Author

scalvert commented May 7, 2021

Looking into how to preserve the commit history, for correct attribution.

@scalvert
Copy link
Collaborator Author

scalvert commented May 7, 2021

Preserving the history is a bit tricky here, since simply adding sarif-sdk as a remote and cherry-picking the commits onto this repo results in consistent conflicts due to the differing structures of the respective repos (the sarif-sdk repo assuming a vs.net solution structure). I'm going to ensure that correct attribution is added via Co-authored-by.

jeffersonking
jeffersonking approved these changes May 8, 2021
View reviewed changes
Copy link
Collaborator

@jeffersonking jeffersonking left a comment

Choose a reason for hiding this comment

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

I did a diff of sarif.js from old and new repos. Appears to be all coding style changes (aka no semantic changes). lgtm.

@scalvert
Copy link
Collaborator Author

scalvert commented May 8, 2021

Correct, @jeffersonking. It was just "Prettier"-fied.

Thanks for the review!

@scalvert scalvert merged commit 8222955 into main May 8, 2021
@scalvert scalvert deleted the move-eslint-formatter branch May 8, 2021 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffersonking jeffersonking jeffersonking approved these changes

@eddynaka eddynaka Awaiting requested review from eddynaka

@GabeDeBacker GabeDeBacker Awaiting requested review from GabeDeBacker GabeDeBacker is a code owner

@michaelcfanning michaelcfanning Awaiting requested review from michaelcfanning michaelcfanning is a code owner

@rwjblue rwjblue Awaiting requested review from rwjblue rwjblue is a code owner

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@scalvert @jeffersonking