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

Use static typing where available #216

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jacksonj04
Copy link
Member

@jacksonj04 jacksonj04 commented Feb 24, 2023

tl;dr: we should try use static typing where it exists, because it helps protect us from easy mistakes and makes us more fully consider the data our code is using.

Bikeshed for this at https://bikeshed.dxw.com/2023/02/24/just-my-type-why-we-should-start-using-more-static-typing/

@github-actions github-actions bot added the new New pull requests needing review label Feb 24, 2023
@jacksonj04 jacksonj04 force-pushed the rfc/use-static-typing-where-available branch 2 times, most recently from c5a0b64 to f922c4d Compare February 24, 2023 09:57
@jacksonj04 jacksonj04 marked this pull request as ready for review February 24, 2023 10:00
@snim2
Copy link
Contributor

snim2 commented Feb 24, 2023

Whilst I'm a big fan of external typing tools like MyPy, I'm a little more cautious about this proposal. Firstly, I think we need to differentiate a little between how we deal with:

  • New code and legacy code
  • Repos we have forked and ones we've created
  • Code that is relatively stand-alone, and codebases where the use of a larger framework or whatever might complicate the use of internal type annotations

I'd be interested to hear any thoughts @matthewpassmore and @serena-piccioni about how we might apply some of the thoughts in this draft to GovPress work.

Copy link
Contributor

@richpjames richpjames left a comment

Choose a reason for hiding this comment

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

I agree with this!
I would go as far as saying any non-trivial Javascript app MUST use typescript but in the interest of keeping this RFC as broad as possible I think SHOULD is fine.

@github-actions github-actions bot removed the new New pull requests needing review label Mar 10, 2023
@dragon-dxw
Copy link

I agree with this! I would go as far as saying any non-trivial Javascript app MUST use typescript but in the interest of keeping this RFC as broad as possible I think SHOULD is fine.

We should probably be explicit about per-language choices, even if they are only recommendations.

| ---------- | -------------------------------- |
| C# | N/A |
| Kotlin | N/A |
| PHP | N/A |
Copy link
Contributor

Choose a reason for hiding this comment

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

@RobjS @matthewpassmore do you have any thoughts on this? I think we probably haven't always been consistent about how we use annotations?

Copy link
Member Author

@jacksonj04 jacksonj04 May 10, 2023

Choose a reason for hiding this comment

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

I left PHP as N/A here because (unlike Ruby/Python/JS) the typing is actually integral to the language rather than needing you to run an extra set of tools to make it work, although because it's interpreted there's probably a good way to include it in CI. If there's a handy PSR for it then it should probably be included in the same vein as PEP 484 though!

Copy link
Contributor

Choose a reason for hiding this comment

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

We've used Psalm for static analysis on some projects. It does more than enforcing typing, but that's a big part of it, and we may be able to configure it to only worry about typing.

I've tried using static typing in PHP before and found it a bit of a mixed bag, as the tools don't always exist within the language to do it properly (although that has improved with PHP8), which means you end up with a combination of types declared in language and types declared in docblocks, which can feel a bit clunky.

Copy link
Member Author

@jacksonj04 jacksonj04 May 22, 2023

Choose a reason for hiding this comment

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

mypy for Python isn't strictly speaking only for typing (although that's certainly its main purpose), and it can surface some other "this is obviously not right" stuff so I don't think recommending Psalm is necessarily a bad idea.

I think my preference would be for in-language type declarations, but that's only really because that's the way PEP 484 does it and consistency is good. No strong feelings if people actually working with PHP every day feel something else is better.

Copy link

@dragon-dxw dragon-dxw Jul 9, 2024

Choose a reason for hiding this comment

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

This PR no longer recommends specific typing tools -- I believe we can resolve this conversation.

@jacksonj04 jacksonj04 force-pushed the rfc/use-static-typing-where-available branch from 290335d to 808423e Compare September 19, 2023 10:34
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