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

Add eslint config for eslint #57

Merged
merged 2 commits into from
Nov 11, 2022
Merged

Add eslint config for eslint #57

merged 2 commits into from
Nov 11, 2022

Conversation

quesabe
Copy link
Collaborator

@quesabe quesabe commented Oct 30, 2022

No description provided.

@quesabe quesabe self-assigned this Oct 30, 2022
@quesabe quesabe requested a review from gvidon October 30, 2022 12:57
@quesabe
Copy link
Collaborator Author

quesabe commented Oct 30, 2022

@gvidon The PR includes both the config and its integration into the ofmt script. You can see that CI fails, which is caused by the config not being published yet. There remains a chance that something goes wrong and it won't work as expected.

I see two options here:

  1. To make it shoother we can separately publish an update with the config only and then when it is available in npm I'd test the config and submit the updated script.
  2. Publish all at once and then do a fix if needed.

Please let me know which route you prefer. PS: for me the first one seems better.

gvidon
gvidon requested changes Oct 30, 2022
View reviewed changes
Copy link
Collaborator

@gvidon gvidon left a comment

Choose a reason for hiding this comment

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

@Serator can you please test the functionality in a real project?

eslint-config-ofmt/eslint.layout.cjs Outdated Show resolved Hide resolved
@quesabe quesabe force-pushed the add-eslint-config-for-eslint branch from 2f8ba3b to 0aa3100 Compare November 1, 2022 19:04
@quesabe quesabe requested a review from gvidon November 4, 2022 05:37
gvidon
gvidon approved these changes Nov 4, 2022
View reviewed changes
Copy link
Collaborator

@gvidon gvidon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@quesabe let's do the second option, seems easier to me. Feel free to merge the PR.

@quesabe quesabe merged commit 48d3e90 into master Nov 11, 2022
@gvidon gvidon deleted the add-eslint-config-for-eslint branch November 12, 2022 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvidon gvidon gvidon approved these changes

Assignees

@quesabe quesabe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@quesabe @gvidon