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

Improved Tooling and Coding Standards #1090

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

Conversation

4np
Copy link

@4np 4np commented Feb 6, 2025

TLDR

This PR improves the tooling, extends the SwiftLint rules and introduces Swift Format rules.

These changes make it easier to get up and running and contribute to the project while introducing a common code style and coding standard (which is currently largely missing).

What changed?

This PR:

  • configures Git hooks rather than copying the Git Hooks (e.g. git config core.hooksPath)
  • updates the SwiftLint rules to better enforce a common standard:
    • changed some _error_s when unsafe logic is used (for example force unwrap, force try, etcetera which may cause crashes) to _warning_s as there were too many errors (for the time being?)
    • opt-in to a bunch of opt-in rules which catch potential issues and / or better enforce a common standard
    • Note: this was reverted in a422d5c as requested in a review comment and the rules are now stored in .swiftlint-proposed.yml for potentially later uptake
  • adds a Swift Format configuration
  • updates the project to use the SwiftLintBuildToolPlugin build phase plugin rather than relying on an externally managed swiftlint installation.
  • updates the pre-commit hook to:
    • format Swift source code using Swift Format (bundled with Xcode)
    • fix lint issues (e..g swiftlint --fix)
    • use SwiftLintBuildToolPlugin's binary swiftlint distribution rather than relying on an externally Homebrew managed SwiftLint installation.
  • externalizes configuration:
    • added Common.xcconfig for shared configuration
    • added Debug.xcconfig for Debug configuration
    • added Release.xcconfig for Release configuration
    • added support for local overrides in Local.xcconfig
    • removed the scripts that update the team and bundle identifiers (e.g. thebenternify.sh and unthebenternify.sh) as that can now be handled through the externalized configuration (they may require updates, I am not sure what the values should be).
  • Adds MeshtasticDebug.entitlements (which is by default not used) which does not contain the following entitlements:
    • WeatherKit
    • CarKit
    • Associated Domains
  • updates README.md with updated instructions
  • adds a header template containing the right copyright and licensing information.

Warning: this change will cause an overwhelming amount of lint warnings! Some of these warnings should really be errors, but as there are just too many these have been changed to warnings. Hopefully this will be the beginning of some thorough clean-ups / rewrites.
image

Why did it change?

After cloning the project you run into compilation failures as development team and bundle identifier are hardcoded and personal / non-premium Apple Developer accounts do not allow some of the entitlements.

Looking at the code, there didn't appear to be a uniform standard, and most of the settings seemed to be optimized for very large external widescreen displays where, IMHO, they should be optimized for MacBook Pro screens to improve readability.

The changes in this PR make it easier to get started and contribute to the project.

Side notes

Aside from the scope of this PR there's room for improvement:

  • no common coding style and coding standard (this is addressed in part with this PR)
  • separate business logic outside of views (using MVVM or another applicable design pattern)
  • split up massive views into smaller views and / or dedicated @ViewBuilders (composition)
  • lots of force tries and force unwraps which have the potential to crash the app
  • no concurrency / thread safety / isolation (let alone Swift 6 language mode)
  • memory leaks / not weakly capturing strong references
  • invalid use of the localization system (invalid grammar, plural rules, gender forms, localization context, etcetera)
    • invalid localized string concatenation and interpolation (causes invalid grammar in non-English languages)
    • no plural rules
    • missing descriptive comments to provide translation context to translators
  • no unit tests
  • etcetera...

How is this tested?

These are primarily non-testable changes.

Screenshots/Videos (when applicable)

If your Apple Developer Account is a personal / non-premium Apple Developer Account, you cannot compile as WeatherKit, CarKit and Associated Domains require a premium subscription. In that case you can update the Debug entitlements to MesthtasticDebug.entitlements which does not contain WeatherKit, CarKit and Associated Domains so it will allow you to compile the app:

image

The updated SwinftLint rules for a common code style / coding standards will unfortunately introduce quite a number of new warnings. Some of these new warnings are really errors, but as there were so many errors they have for now been downgraded to warnings:
image

Added a logo.png for Sourcetree Git client:
image

Added a header template containing the right copyright and license information:

image

Checklist

  • My code adheres to the project's coding and style guidelines. (this PR adds coding standards)
  • I have conducted a self-review of my code.
  • I have commented my code, particularly in complex areas.
  • I have made corresponding changes to the documentation.
  • I have tested the change to ensure that it works as intended.

4np added 3 commits February 6, 2025 11:42
…'s SwiftFormat, updated lint rules and other improvements.
…d remove the 'thebentern' scripts as that can now be handed in the Local.xcconfig local overrides.
…rKit and Associated Domains so you can build without issues using a non-premium or Personal Developer Account.
@CLAassistant
Copy link

CLAassistant commented Feb 6, 2025

CLA assistant check
All committers have signed the CLA.

@4np 4np changed the title Improved Tooling and formatting Improved Tooling and Coding Standards Feb 7, 2025
@4np 4np marked this pull request as ready for review February 7, 2025 08:33
@garthvh
Copy link
Member

garthvh commented Feb 11, 2025

This looks good, can you split the swift lint stuff out?

@4np
Copy link
Author

4np commented Feb 13, 2025

Hi @garthvh ,

I assume with the SwiftLint stuff you mean the SwiftLint configuration that's causing the 3855 warnings? Sure, I can revert to the previous lint rules.

I realize the number of warnings can be overwhelming, just note though that there are warnings (errors) in there that flag problematic or crash prone code, which you'll miss out on.

… and keep the new proposed rules as '.swiftlint-proposed.yml'.
@4np
Copy link
Author

4np commented Feb 13, 2025

In the above ☝️ commit I have:

  • reverted the SwiftLint rules to the original .swiftlint.yml rules
  • stored the new rules I originally added as .swiftlint-proposed.yml
  • updated the pre-commit hook to validate the pbxproj file does not contain the MeshtasticDebug.entitlements, which shouldn't be committed.

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