Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions org/allPRs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,6 @@ export const singletons = async () => {
...danger.git.created_files
].filter(file => file.endsWith(".swift"));

// If no files to check after filtering, exit early
if (changedFiles.length === 0) {
return;
}

for (const file of changedFiles) {
let diff = await danger.git.diffForFile(file);
let addedLines = diff?.added.split(/\n/);
Expand All @@ -101,6 +96,25 @@ export const singletons = async () => {
}
}

export const userDefaultsWrapper = async () => {
const changedFiles = [
...danger.git.modified_files,
...danger.git.created_files
].filter(file => file.endsWith(".swift"));

for (const file of changedFiles) {
let diff = await danger.git.diffForFile(file);
let addedLines = diff?.added.split(/\n/);
const foundOccurrence = addedLines?.find(value => /^\+(?!\s*\/\/)\s*@UserDefaultsWrapper(?:\((?:.*\))?)?$/.test(value));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Regex misses single-line property wrapper declarations

Medium Severity

The regex /^\+(?!\s*\/\/)\s*@UserDefaultsWrapper(?:\((?:.*\))?)?$/ requires @UserDefaultsWrapper to be at the end of the line due to the $ anchor. This means valid Swift declarations where the wrapper and property are on the same line (e.g., @UserDefaultsWrapper var myProperty: Bool or @UserDefaultsWrapper(key: .foo) var myProperty: Bool) won't be detected. Developers could bypass the check by using single-line syntax instead of placing the attribute on its own line.


Please tell me if this was useful or not with a 👍 or 👎.

Fix in Cursor Fix in Web

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is enough for now, we only use UserDefaultsWrapper on a separate line.

if (foundOccurrence) {
// trim leading + and whitespace
const cleanLine = foundOccurrence.replace(/^\+\s*/, '').trim();
fail(`New \`@UserDefaultsWrapper\` definitions are not allowed. Please use \`KeyValueStoring\` protocol instead.\nFound this line:\n\`\`\`swift\n${cleanLine}\n\`\`\``);
return;
}
}
}

export const localizedStrings = async () => {
for (let file of danger.git.modified_files) {
let diff = await danger.git.diffForFile(file);
Expand Down Expand Up @@ -235,6 +249,7 @@ export default async () => {
await internalLink()
await xcodeprojConfiguration_macOS()
await singletons()
await userDefaultsWrapper()
await localizedStrings()
await licensedFonts()
await newColors()
Expand Down
127 changes: 127 additions & 0 deletions tests/userDefaultsWrapper.allPRs.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
jest.mock("danger", () => jest.fn())
import danger from 'danger'
const dm = danger as any;

import { userDefaultsWrapper } from '../org/allPRs'

beforeEach(() => {
dm.addedLines = ""
dm.fail = jest.fn().mockReturnValue(true);

dm.danger = {
git: {
diffForFile: async (_filename) => {
return { added: dm.addedLines }
},
modified_files: [
"ModifiedFile.swift"
],
created_files: [
"CreatedFile.swift"
]
}
}
})

describe("@UserDefaultsWrapper checks", () => {
it("does not fail with no changes to Swift files", async () => {
dm.danger.git.modified_files = []

await userDefaultsWrapper()
expect(dm.fail).not.toHaveBeenCalled()
})

it("does not fail with no diff in Swift files", async () => {
dm.danger.git.diffForFile = async (_filename) => {}

await userDefaultsWrapper()
expect(dm.fail).not.toHaveBeenCalled()
})

it("does not fail with no additions", async () => {
await userDefaultsWrapper()
expect(dm.fail).not.toHaveBeenCalled()
})

it("does not fail with non-@UserDefaultsWrapper additions", async () => {
dm.addedLines = `
+ let subscription = PrivacyProSubscription(status: .inactive)
+ let isSubscribed: Bool
+ var billingPeriod: String?
+ public let startedAt: Int?
+ internal let expiresOrRenewsAt: Int?
+ private var paymentPlatform: String?
+ let status: String?
`

await userDefaultsWrapper()
expect(dm.fail).not.toHaveBeenCalled()
})

it("does not fail with UserDefaultsWrapper usage additions", async () => {
dm.addedLines = `
+ let userDefaults: UserDefaults = UserDefaultsWrapper<Any>.sharedDefaults
`

await userDefaultsWrapper()
expect(dm.fail).not.toHaveBeenCalled()
})

it("does not fail with commented out @UserDefaultsWrapper additions", async () => {
dm.addedLines = `
+ // @UserDefaultsWrapper(key: .homePageIsFavoriteVisible, defaultValue: true)
`

await userDefaultsWrapper()
expect(dm.fail).not.toHaveBeenCalled()
})

it("fails with @UserDefaultsWrapper additions", async () => {
dm.addedLines = `
+ @UserDefaultsWrapper(key: .homePageIsFavoriteVisible, defaultValue: true)
`

await userDefaultsWrapper()
expect(dm.fail).toHaveBeenCalled()
})

it("fails with @UserDefaultsWrapper additions with open bracket only", async () => {
dm.addedLines = `
+ @UserDefaultsWrapper(
+ key: .homePageIsFavoriteVisible,
+ defaultValue: true
+ )
`

await userDefaultsWrapper()
expect(dm.fail).toHaveBeenCalled()
})

it("fails with @UserDefaultsWrapper additions without parameters", async () => {
dm.addedLines = `
+ @UserDefaultsWrapper
+ private var didCrashDuringCrashHandlersSetUp: Bool
`

await userDefaultsWrapper()
expect(dm.fail).toHaveBeenCalled()
})

it("does not fail with @UserDefaultsWrapper deletions", async () => {
dm.addedLines = `
- @UserDefaultsWrapper
- private var didCrashDuringCrashHandlersSetUp: Bool
-
- @UserDefaultsWrapper(
- key: .homePageIsFavoriteVisible,
- defaultValue: true
- )
-
- @UserDefaultsWrapper(key: .homePageIsFavoriteVisible, defaultValue: true)
- var isFavoriteVisible: Bool
`

await userDefaultsWrapper()
expect(dm.fail).not.toHaveBeenCalled()
})
})