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: update eslint config #80

Merged

Conversation

Akirtovskis
Copy link
Contributor

Description

Issue briefly discussed : #77

  1. First of many PRs to improve the subdomain registrar performance and Dev experience.
    This is part of M1 of : Subdomain registrar improvement stacksgov/grants-program#282
  2. Adds prettier config and formats the files
  3. Uses the same lint config as all the other Stacks projects

Type of change

Config update | code formatting improvement

Does this introduce a breaking change?

No breaking change!

@codecov
Copy link

codecov bot commented Mar 2, 2022

Codecov Report

Merging #80 (6b120aa) into master (da2d144) will increase coverage by 0.02%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
+ Coverage   58.19%   58.22%   +0.02%     
==========================================
  Files           8        8              
  Lines         665      663       -2     
==========================================
- Hits          387      386       -1     
+ Misses        278      277       -1     
Impacted Files Coverage Δ
src/db.js 89.47% <88.46%> (+0.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da2d144...6b120aa. Read the comment docs.

@Akirtovskis
Copy link
Contributor Author

@CharlieC3 this is the first PR updating the lint config according to patterns used in other Stacks projects.

Still facing some flow issues though in the CI. Do you have any idea what might be wrong here? 👀

@CharlieC3
Copy link
Member

@wileyj

@Akirtovskis
Copy link
Contributor Author

@CharlieC3 @wileyj any more pointers on how to go about it? 👀

@Akirtovskis
Copy link
Contributor Author

@CharlieC3 @wileyj any more pointers on how to go about it? 👀

@CharlieC3
Copy link
Member

@Akirtovskis Looks like flow is checking some files in the node_modules dir and erroring on some files that intentionally have bad syntax:
https://github.com/stacks-network/subdomain-registrar/pull/80/checks#step:6:50

I would suggest updating the .flowconfig file to ignore performing static analysis on the node_modules directory.

@Akirtovskis
Copy link
Contributor Author

@CharlieC3 @wileyj that indeed was the case.

Sorry that it took a bit more time to figure this out due to me first time touching flow 😃

feat: fix eslint config

fix: cleanup

fix: temp remove flow types

feat: remove src from flow ignore

fix: revert flow config

fix: revert changes of config

fix: remove the extra line

feat: fix with removing node_modules from flow

fix: upd ignore config of flow

fix: update flow

fix: upd node modules

fix: upd flow config once more

fix: flowconfig ignore updates

fix: update flowconfig ignore
@Akirtovskis
Copy link
Contributor Author

@CharlieC3 any other feedbacks from you or are we good to go for this one?

@CharlieC3
Copy link
Member

@Akirtovskis I'm not a primary maintainer for this repo, I think @asimm241 should be able to help you get it merged.

@Akirtovskis
Copy link
Contributor Author

@asimm241 hey hey! Did you have a chance to look at this?

1 similar comment
@Akirtovskis
Copy link
Contributor Author

@asimm241 hey hey! Did you have a chance to look at this?

@wileyj
Copy link
Contributor

wileyj commented Mar 23, 2022

Sorry @Akirtovskis i missed the @ for me here!
I don't see any issues with this PR, but i'm also not familiar with the code personally - let's give @asimm241 another day to respond.

I'm inclined to approve this since it's not changing any of the base code.

@asimm241
Copy link
Contributor

Sorry about the delay guys, This skipped my attention. Thanks for bumping it up. I'll start on it tomorrow.

module.exports = {
root: true,
parser: "@babel/eslint-parser",
plugins: ["eslint-plugin-prettier"],
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to apply prettier to the flow files or just the typescript ones?

.eslintrc.js Show resolved Hide resolved
node: true,
commonjs: true,
},
parserOptions: {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this go inside the overrides section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There have to be both parser options, otherwise pipeline fail with some config error related to that.

Copy link
Contributor

@asimm241 asimm241 left a comment

Choose a reason for hiding this comment

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

We might want to revisit eslintrc.js once all the files migrated to .ts. Otherwise LGTM!

@Akirtovskis
Copy link
Contributor Author

@asimm241 updated TS version as you requested and pipelines green now, so should be good to 🚀

@wileyj wileyj merged commit 1a1ba9a into stacks-network:master Apr 1, 2022
@blockstack-devops
Copy link

🎉 This PR is included in version 1.1.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants