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 static code analysis #1970

Merged
merged 30 commits into from
May 13, 2019
Merged

Add static code analysis #1970

merged 30 commits into from
May 13, 2019

Conversation

compulim
Copy link
Contributor

@compulim compulim commented May 8, 2019 •
edited
Loading

Changelog Entry

Added

Description

Adding code analysis tool to our build to make the code easier to maintain.

Also named all React components, makes debugging in React extension is slightly easier.

Although there are lot of files changes, most of them are:

  • Adding defaultProps and propTypes
  • Refactoring functional component as a separate function for naming
  • Reordering functions to meet no-use-before-define rule
  • Remove extra parenthesis

Specific Changes

  • Added eslint and related packages
  • Enabled eslint:recommended rules and react:recommended rules
  • In addition to recommended rules, hand-picked more rules
    • 144 additional rules were enabled, on top of the recommended set of 69 rules
    • No stylistic rules were picked
  • Name all React components, no more <Unknown> component in React dev tools
  • Add eslint to CI/CD pipeline
  • Testing Added

@compulim compulim requested a review from corinagum May 8, 2019 09:10
corinagum
corinagum suggested changes May 9, 2019
View reviewed changes
Copy link
Contributor

@corinagum corinagum 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 few comments - some of them are questions because I don't understand. This looks really great for iteration #1.

.eslintrc.yml Show resolved Hide resolved
.eslintrc.yml Show resolved Hide resolved
.eslintrc.yml Show resolved Hide resolved
packages/component/package.json Show resolved Hide resolved
packages/component/src/Activity/CarouselLayout.js Outdated Show resolved Hide resolved
packages/component/src/Composer.js Outdated Show resolved Hide resolved
packages/component/src/connectToWebChat.js Show resolved Hide resolved
packages/core/src/__tests__/connectSaga.spec.js Show resolved Hide resolved
@compulim compulim marked this pull request as ready for review May 13, 2019 18:36
@compulim compulim merged commit 45b7f25 into microsoft:master May 13, 2019
@compulim compulim mentioned this pull request Jul 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corinagum corinagum corinagum approved these changes

@a-b-r-o-w-n a-b-r-o-w-n Awaiting requested review from a-b-r-o-w-n a-b-r-o-w-n is a code owner

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

@justinwilaby justinwilaby Awaiting requested review from justinwilaby

@tonyanziano tonyanziano Awaiting requested review from tonyanziano

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@compulim @corinagum