Skip to content

Comments

Adds automated linting#2382

Merged
elland merged 1 commit intodevelopfrom
hlint-stern
May 25, 2022
Merged

Adds automated linting#2382
elland merged 1 commit intodevelopfrom
hlint-stern

Conversation

@elland
Copy link
Contributor

@elland elland commented May 11, 2022

In the interest of keeping things automated, consistent and avoiding repetition, we want to automate linting.

As it can be a contentious topic, this PR will serve as both a battleground for bikeshedding the linter rules as well as the de-facto linting process, whereas we incrementally apply the newly defined rules to the whole codebase.

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.

@elland elland temporarily deployed to cachix May 11, 2022 08:32 Inactive
@elland elland temporarily deployed to cachix May 11, 2022 08:34 Inactive
@elland elland temporarily deployed to cachix May 11, 2022 08:41 Inactive
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

i have a change request, but i'll approve anyway in the hope that you'll follow it anyway before merging. if you disagree i'll find out how strongly i feel about this. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm always surprised that there is no alternative to not . null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Also sounds like that's a hard naming problem. Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

isJust . nonEmpty ? ;)

isJust . uncons ?

Copy link
Contributor Author

@elland elland May 12, 2022

Choose a reason for hiding this comment

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

How about isNotEmpty = not . null? Or to keep it consistent: notNull = not . null :D

Copy link
Contributor

@smatting smatting left a comment

Choose a reason for hiding this comment

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

+1 for keeping redundant $ and do. All the other changes look like an improvement to me.
Maybe apply hlint to more code to see more examples?

@elland elland temporarily deployed to cachix May 11, 2022 12:06 Inactive
@elland elland temporarily deployed to cachix May 12, 2022 06:46 Inactive
@elland elland temporarily deployed to cachix May 12, 2022 07:28 Inactive
@elland elland temporarily deployed to cachix May 12, 2022 07:38 Inactive
Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

Generally looks like good improvements. See in-line comments.

@elland elland temporarily deployed to cachix May 12, 2022 12:34 Inactive
@elland elland temporarily deployed to cachix May 12, 2022 13:08 Inactive
Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

So can we have a way to ignore enforcement of point-free style in .hlint?

Also @elland the changes here don't pass ormolu (linters step on CI); which suggests that your editor isn't set up to run ormolu on file save. You can manually run make format, but I'd suggest to hook ormolu into your editor setup. That's documented under /docs/developer somewhere.

@elland elland temporarily deployed to cachix May 16, 2022 07:10 Inactive
@elland elland temporarily deployed to cachix May 16, 2022 07:28 Inactive
@elland elland temporarily deployed to cachix May 16, 2022 13:32 Inactive
@elland elland temporarily deployed to cachix May 17, 2022 07:43 Inactive
@elland elland temporarily deployed to cachix May 17, 2022 07:57 Inactive
@elland elland changed the title Added basic linting to stern, as a testing ground. Adds automated linting May 17, 2022
@elland elland temporarily deployed to cachix May 17, 2022 08:02 Inactive
@elland elland temporarily deployed to cachix May 17, 2022 08:57 Inactive
@elland elland temporarily deployed to cachix May 19, 2022 09:40 Inactive
@elland elland temporarily deployed to cachix May 19, 2022 09:47 Inactive
@elland elland temporarily deployed to cachix May 23, 2022 07:54 Inactive
@jschaul
Copy link
Member

jschaul commented May 23, 2022

Could we get the previously discussed and approved changeset (squash-) merged, and all follow-up changes made in a separate PR, please? It's very hard to review an ever-growing PR again and again.

@elland
Copy link
Contributor Author

elland commented May 23, 2022

@jschaul I separated them per commits, so only the latest commit needs review, but I can break it down into PRs as well.

@elland elland temporarily deployed to cachix May 23, 2022 11:54 Inactive
@smatting smatting self-requested a review May 25, 2022 17:11
@elland elland merged commit 85ec135 into develop May 25, 2022
@elland elland deleted the hlint-stern branch May 25, 2022 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants