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 support for arbitrary single-letter tags in Filter #154

Merged
merged 5 commits into from
May 19, 2024
Merged

Conversation

@tyiu tyiu requested review from joelklabo and bryanmontz May 5, 2024 03:21
@tyiu tyiu force-pushed the tyiu/tag-filters branch 2 times, most recently from ad9eae2 to 30ca55c Compare May 5, 2024 03:22
public let pubkeys: [String]?


/// a list of tag values that are referenced by single English-alphabet letter tag names
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// a list of tag values that are referenced by single English-alphabet letter tag names
/// a list of tag values that are referenced by single Latin-script letter tag names

https://en.wikipedia.org/wiki/Latin_script

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to "basic Latin letter" instead.
https://en.wikipedia.org/wiki/ISO_basic_Latin_alphabet

Sources/NostrSDK/Filter.swift Outdated Show resolved Hide resolved

var intValue: Int? { nil }

init?(intValue: Int) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of an initializer that always returns nil?

Copy link
Contributor Author

@tyiu tyiu May 19, 2024

Choose a reason for hiding this comment

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

Its declaration is required by the CodingKey protocol. The compiler doesn't like it if it's not there. Per the documentation of this initializer, it says

    /// Creates a new instance from the specified integer.
    ///
    /// If the value passed as `intValue` does not correspond to any instance of
    /// this type, the result is `nil`.
    ///
    /// - parameter intValue: The integer value of the desired key.
    init?(intValue: Int)

@@ -54,19 +63,85 @@ public struct Filter: Codable, Hashable, Equatable {
/// - kinds: a list of a kind numbers
/// - events: a list of event ids that are referenced in an "e" tag
/// - pubkeys: a list of pubkeys that are referenced in a "p" tag
/// - tags: a list of tag values that are referenced by single English-alphabet letter tag names
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// - tags: a list of tag values that are referenced by single English-alphabet letter tag names
/// - tags: a list of tag values that are referenced by single Latin-script letter tag names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to "basic Latin letter" instead.
https://en.wikipedia.org/wiki/ISO_basic_Latin_alphabet

/// If `tags` contains an `e` tag and `events` is also provided, `events` takes precedence.
/// If `tags` contains a `p` tag and `pubkeys` is also provided, `pubkeys` takes precedence.
///
/// Returns `nil` if `tags` contains tag names that are not in the English-alphabet of A-Z or a-z.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Returns `nil` if `tags` contains tag names that are not in the English-alphabet of A-Z or a-z.
/// Returns `nil` if `tags` contains tag names that are not in the Latin script of A-Z or a-z.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to "basic Latin letter" instead.
https://en.wikipedia.org/wiki/ISO_basic_Latin_alphabet

}

private extension Character {
var isEnglishLetter: Bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO we should generally use "Latin script" or "Latin alphabet" rather than "English letter" because it is more accurate. There are many languages that use the Latin alphabet of A-Z.

Copy link
Contributor Author

@tyiu tyiu May 19, 2024

Choose a reason for hiding this comment

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

I think "Latin script" might be too broad.

The Latin script is the basis of the International Phonetic Alphabet, and the 26 most widespread letters are the letters contained in the ISO basic Latin alphabet, which are the same letters as the English alphabet.

Maybe isBasicLatinLetter is better. I chose Letter over Alphabet or Script because Swift already has an isLetter property, so making the naming consistent to that makes more sense.

@tyiu tyiu requested a review from bryanmontz May 19, 2024 12:45
@bryanmontz
Copy link
Collaborator

@tyiu Feel free to merge this one after resolving conflicts.

@tyiu tyiu merged commit 4277478 into main May 19, 2024
4 checks passed
@tyiu tyiu deleted the tyiu/tag-filters branch May 19, 2024 21:01
RandyMcMillan pushed a commit to RandyMcMillan/nostr-sdk-ios that referenced this pull request Sep 1, 2024
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.

2 participants