Skip to content

feat: exists - #223

Merged
loeffel-io merged 13 commits into
masterfrom
feature/loeffel-io/#30-exists
Jul 11, 2024
Merged

feat: exists#223
loeffel-io merged 13 commits into
masterfrom
feature/loeffel-io/#30-exists

Conversation

@loeffel-io

@loeffel-io loeffel-io commented Apr 23, 2024

Copy link
Copy Markdown
Owner

close #30
close #201
close #202
close #39
close #32
close #234

@loeffel-io loeffel-io self-assigned this Apr 23, 2024
Comment thread internal/linter/linter.go Outdated
defer func() {
fmt.Printf("-----------------------------\nstatistics\n-----------------------------\n")
fmt.Printf("time: %d ms\n", time.Since(linter.GetStatistics().Start).Milliseconds())
fmt.Printf("time: %d µs / %d ms\n", time.Since(linter.GetStatistics().Start).Microseconds(), time.Since(linter.GetStatistics().Start).Milliseconds())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
fmt.Printf("time: %d µs / %d ms\n", time.Since(linter.GetStatistics().Start).Microseconds(), time.Since(linter.GetStatistics().Start).Milliseconds())
fmt.Printf("time: %s\n", time.Since(linter.GetStatistics().Start).Truncate(time.Microsecond).String()

Comment thread cmd/ls_lint/main.go Outdated
filesystem = mapFilesystem
}

log.Printf("%+v", paths)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Left behind?

Comment thread internal/rule/exists.go
var err error

if value, err = strconv.ParseInt(params[0], 10, 16); err != nil {
return err.(*strconv.NumError).Err

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You should return err

And use errors.As to catch them

Or you use a type assertion check

Suggested change
return err.(*strconv.NumError).Err
if nerr, ok := err.(*strconv.NumError); ok {
return nerr.Err
}
return err

I'm a bit worried by the possible panic here if strconv slightly changes

Comment thread internal/rule/exists.go

func (rule *Exists) GetParameters() []string {
if rule.getMin() == rule.getMax() {
return []string{fmt.Sprintf("%d", rule.getMin())}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
return []string{fmt.Sprintf("%d", rule.getMin())}
return []string{fmt.Sprint(rule.getMin())}

Comment thread internal/rule/exists.go Outdated
type Exists struct {
name string
exclusive bool
min int16

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You are using a limited int, maybe for memory considerate. I'm unsure.

I would have used int.

But if you are looking for performance or memory footprint maybe you could use uint16

Suggested change
min int16
min uint16

@loeffel-io

Copy link
Copy Markdown
Owner Author

Thanks @ccoVeille for your review 😄 this pr is in very early wip - i will let you know when its ready to review ❤️

@loeffel-io loeffel-io mentioned this pull request Jul 11, 2024
@ccoVeille

Copy link
Copy Markdown

I don't think k this would bring support for

@ccoVeille

ccoVeille commented Jul 11, 2024

Copy link
Copy Markdown

Thanks @ccoVeille for your review 😄 this pr is in very early wip

I'm a compulsive reviewer. I know I should respect the "no review on draft" rule.

But sometimes (often) maintainers are leaving something in draft for days/weeks, then when it could be ready to be reviewed, they click merge.

I would expect maintainers to comply the "wait 24-48h for code to be reviewed" rule 😅

@loeffel-io
loeffel-io marked this pull request as ready for review July 11, 2024 16:45
@loeffel-io
loeffel-io merged commit 888a537 into master Jul 11, 2024
@loeffel-io
loeffel-io deleted the feature/loeffel-io/#30-exists branch July 11, 2024 16:46
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.

Support "none" rule Feature Proposal: Blocklists How to disable some specific names? Strict mode? Rule: Exists

2 participants