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

Standardize imports #2477

Draft
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

RichDom2185
Copy link
Member

@RichDom2185 RichDom2185 commented May 12, 2023

Description

Motivation: Copied over from their own documentation

Moving a file to different folder, could result in changing all imports statement in that file. This will not happen is the import paths are absolute.

Also, the current way of importing leads to stuff like

import { a } from 'src/.../file'

import { b } from '../file'

Even though a and b are defined in the same file, which does not really look nice at best and potentially misleading at worst.

Changes:

  • Migrated .eslintrc from JSON to a JS file to allow for greater readability when writing rules
  • Added no-relative-import-paths plugin
  • Enforced absolute imports for files that are not test files (reason being test files generally import their SUT from their siblings/cousins)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Code quality improvements

How to test

Checklist

  • I have tested this code
  • I have updated the documentation

@RichDom2185 RichDom2185 requested a review from ianyong May 12, 2023 17:45
@RichDom2185 RichDom2185 self-assigned this May 12, 2023
@RichDom2185
Copy link
Member Author

Gonna leave this as draft for a while because the changes across the files might cause merge conflicts with other PRs; let us merge as many of them in as possible first.

Copy link
Member

@ianyong ianyong left a comment

Choose a reason for hiding this comment

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

LGTM! Only checked the changes made by hand in detail; briefly scrolled through the linter/formatter changes and they seem to be correct.

Just one minor clarification.

'@typescript-eslint/explicit-module-boundary-types': 'off',
'@typescript-eslint/ban-ts-comment': 'warn',
'@typescript-eslint/ban-types': 'off',
'no-relative-import-paths/no-relative-import-paths': ['error', { allowSameFolder: true }],
Copy link
Member

Choose a reason for hiding this comment

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

Motivation: Copied over from their own documentation

Moving a file to different folder, could result in changing all imports statement in that file. This will not happen is the import paths are absolute.

Also, the current way of importing leads to stuff like

import { a } from 'src/.../file'

import { b } from '../file'

Even though a and b are defined in the same file, which does not really look nice at best and potentially misleading at worst.

Hmm, given the motivation for this change, is there a reason why the allowSameFolder option is set to true? More specifically, why are relative import paths for imported files from the same folder allowed? Moving a file containing such relative import paths will result in these import statements changing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking that it might encourage better organisation of code (e.g perhaps splitting a 1000+ line component into multiple files in the same folder, where applicable). Since this makes the distinction clear (whether the files are closely related or not), this might improve readability.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

No strong preferences! Just wanted to know if there was a rationale (which you do have) because it would be good to state it somewhere for future reference.

@coveralls
Copy link

coveralls commented May 13, 2023

Pull Request Test Coverage Report for Build 5219070096

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 37.881%

Totals Coverage Status
Change from base Build 5219065445: 0.0%
Covered Lines: 5349
Relevant Lines: 13243

💛 - Coveralls

@chownces
Copy link
Contributor

Refer to #1376

@RichDom2185
Copy link
Member Author

Refer to #1376

Thanks! Is there a way to lint this, though? I tried searching for eslint plugins but couldn't find anything that offers this level of customisation

Must have been added unintentionally following all the merges.
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.

4 participants