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

server: make more use of hlint #6059

Merged
merged 19 commits into from
Oct 28, 2020
Merged

server: make more use of hlint #6059

merged 19 commits into from
Oct 28, 2020

Conversation

abooij
Copy link
Contributor

@abooij abooij commented Oct 22, 2020

Description

#5957 added an hlint code quality checking phase to the CI. However, as described in that PR, we left many hints disabled, as they may be considered controversial. This PR re-enables a number of hints, hopefully in a non-controversial way. Future PRs will be checked against them.

Using the hlint "refactor" tool, I have also applied many of the changes suggested by hlint. However, this tool needs to be babysit somewhat: it can generate code that does not compile, such as the following instance of the "Redundant bracket" hint:

-buildPostgresSpecs :: (HasVersion) => RawConnInfo -> IO Spec
+buildPostgresSpecs :: (HasVersion => RawConnInfo -> IO Spec
 buildPostgresSpecs pgConnOptions = do
   env <- getEnvironment

and it can remove comments, such as the following instance of the "Functor law" hint:

         runExceptT
         . withExceptT qeCode -- just look at Code for purposes of tests
-        . fmap _uiRole -- just look at RoleName for purposes of tests
-        . fmap fst -- disregard Nothing expiration
-        . getUserInfoWithExpTime_ userInfoFromAuthHook processJwt () () rawHeaders
+        . fmap (_uiRole . fst) . getUserInfoWithExpTime_ userInfoFromAuthHook processJwt () () rawHeaders
         where
           -- mock authorization callbacks:

(lines starting with - are removed by hlint; lines starting with + are added)

I have gone through the code after hlint's refactoring was applied, to make sure not too strange code is generated. However, please do feel free to scrutinize the changes introduced by this PR. (Actually, the first commit in this PR, e09db0c, which only enables hlint hints, does not pass CI. This is due to a mistake in the way the hlint script is run, and I have implemented a fix for this as part of #6003 where I already encountered this.)

As a reminder, the fact that this PR both enables hints and implements them is a choice: #5957 only runs hlint on files that were changed by a PR.

Changelog

  • CHANGELOG.md is updated with user-facing content relevant to this PR. If no changelog is required, then add the no-changelog-required label.

Affected components

  • Server
  • Tests

Related Issues

Solution and Design

Steps to test and verify

Limitations, known bugs & workarounds

Server checklist

Catalog upgrade

Does this PR change Hasura Catalog version?

  • No

Metadata

Does this PR add a new Metadata feature?

  • No

GraphQL

  • No new GraphQL schema is generated

Breaking changes

  • No Breaking changes

@abooij abooij added the c/server Related to server label Oct 22, 2020
@nicuveo
Copy link
Contributor

nicuveo commented Oct 22, 2020

This PR should not touch bench-wrk, which is not code that we own (AFAICT?).

@abooij abooij marked this pull request as ready for review October 22, 2020 20:53
@abooij abooij requested a review from a team as a code owner October 22, 2020 20:53
@abooij abooij requested a review from 0x777 October 23, 2020 07:34
@netlify
Copy link

netlify bot commented Oct 28, 2020

Deploy preview for hasura-docs ready!

Built with commit fa5fc48

https://deploy-preview-6059--hasura-docs.netlify.app

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.

4 participants