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

Add external test library modules to import ordering in style doc #6

Closed
wants to merge 1 commit into from

Conversation

Jimbo4350
Copy link
Collaborator

No description provided.

@@ -254,7 +254,8 @@ Imports should be grouped in the following order:
1. External modules.
2. External IOHK modules.
3. Local library modules.
4. Local test modules
4. Local test modules.
5. External testing library modules.
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't the order be reversed similarly to non-test modules i.e.:

4. External testing library modules.
5. Local test modules.

@newhoggy
Copy link
Collaborator

newhoggy commented May 18, 2023

The downside to having import groups like this in the style guide is that what ends up happening is PRs get block in reviews purely for import grouping reasons or otherwise get missed by the coder and reviewer and get merged anyway. One affects our velocity as PRs that are otherwise ready don't get merged and we end up doing more rebases and CI jobs whilst the other means more of our code doesn't match the style guide.

I have a counter-proposal:

  • Delete the section on import group
  • Add a section that our code must pass stylish-haskell checks if there isn't already one
  • Add import grouping rules to our stylish-haskell config to enforce the style we want We write [import grouping rules]
  • Any import grouping rules we cannot express in the stylish-haskell configuration are discarded.
  • Make stylish-haskell a required check

This has the following advantages:

  • All future merges to the master are guaranteed to follow our import grouping rules.
  • Contributors get much faster feedback and don't have to wait for a review to realise their import grouping fails the style guide
  • Reviewers have one less thing to worry about making reviews much faster.
  • PRs get merged faster and suffer fewer rebases and conflicts
  • If stylish-haskell is enabled locally, the local file will automatically match the style guide relieving the developer of one more thing to think about

@newhoggy newhoggy requested a review from carbolymer May 19, 2023 01:00
@Jimbo4350
Copy link
Collaborator Author

Closing this PR as I agree with John: #6 (comment)

@Jimbo4350 Jimbo4350 closed this May 19, 2023
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.

3 participants