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

Hk cyclomatic complexity #92

Merged
merged 14 commits into from Feb 16, 2019
Merged

Hk cyclomatic complexity #92

merged 14 commits into from Feb 16, 2019

Conversation

hanneskaeufler
Copy link
Contributor

Adresses #91. Just fyi that I tried this. Uses the same logic as rubocop does. I might go ahead and extract this CountingVisitor into a separate file and add more test coverage. I kind of works 🤷🏼‍♂️ Was easy, mad props on the architecture!

veelenga
veelenga reviewed Feb 16, 2019
View reviewed changes
Copy link
Member

@veelenga veelenga left a comment •
edited

Choose a reason for hiding this comment

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

Nice.

  1. Please move files to metrics directory (instead of metric)
  2. Would love to see some more tests covering all code blocks separately. But that’s optional, i can merge and add them by my self if you wish

@hanneskaeufler
Refactor specs to use a macro
25de8ae
@hanneskaeufler
Indent code inside macro
189a556
@hanneskaeufler
Replace array with tuple for string formatting.
2351513 
`Tuple` is stack based, whereas `Array` is allocated on the heap increasing GC pressure.
@hanneskaeufler hanneskaeufler marked this pull request as ready for review February 16, 2019 15:29
@hanneskaeufler
Copy link
Contributor Author

I think this is now ready for review, thanks for the comments!

@Sija
Copy link
Member

Sija commented Feb 16, 2019

crystal tool format :)

Sija
Sija reviewed Feb 16, 2019
View reviewed changes
Copy link
Member

@Sija Sija left a comment

Choose a reason for hiding this comment

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

Ouch, that indentation rly sucks, looks like a formatter bug to me...

@hanneskaeufler
Copy link
Contributor Author

Yes looks pretty horrible, but oh well 🤷🏼‍♂️

@veelenga
Copy link
Member

FYI one of the most complex methods in crystal repo (level of complexity: 513)

https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/syntax/lexer.cr#L90-L1229

@Sija
Copy link
Member

Sija commented Feb 16, 2019

@veelenga whooaaa, I thought that scrolling will never end...

@hanneskaeufler
Copy link
Contributor Author

hanneskaeufler commented Feb 16, 2019 •
edited

FYI one of the most complex methods in crystal repo (level of complexity: 513)

https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/syntax/lexer.cr#L90-L1229

Haha that's a good one 🤣

Btw 10 as the threshold was just a wild guess. Rubocop has is set to 6, swiftlint warns at 10, errors at 20. Some python tool says 10 is still a B. Credos default is at 9, I'd be fine with 10 as a default!

@veelenga veelenga merged commit e850bff into crystal-ameba:master Feb 16, 2019
@veelenga
Copy link
Member

Thank you guys. Good work

@hanneskaeufler hanneskaeufler deleted the hk-cyclomatic-complexity branch February 16, 2019 19:05
@Sija Sija added the rule label Oct 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sija Sija Sija left review comments

@veelenga veelenga veelenga left review comments

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@hanneskaeufler @Sija @veelenga