Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Split "filters" files #36

Merged
merged 13 commits into from
May 1, 2017
Merged

Split "filters" files #36

merged 13 commits into from
May 1, 2017

Conversation

AliSoftware
Copy link
Contributor

@AliSoftware AliSoftware commented Apr 30, 2017

  1. Make files better organized, changing struct namespaces to enums and renaming some files
  2. Also move types into nested types to make it more swifty and better namespaced.
    Old names for those types still exists as typealiases but marked as deprecated and will be removed by Remove deprecated, non nested error types #37
  3. Also uniformized documentation in the process

Copy link
Member

@djbe djbe left a comment

Choose a reason for hiding this comment

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

Nice! I'm assuming there were no code changes beyond the error types.

Just noticed a couple of typos + some inconsistencies in the doc blocks. In some blocks this isn't done:

 - Parameters:
   - in: ...

Note: I can do these tomorrow, right now I'm too tired from the return trip from the weekend.

typealias Parameter = (key: String, value: Any)
public typealias StringDict = [String: Any]

/// Transforms a list of strings representing structred-key/value pairs, like
Copy link
Member

Choose a reason for hiding this comment

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

structured

typealias Parameter = (key: String, value: Any)
public typealias StringDict = [String: Any]

/// Transforms a list of strings representing structred-key/value pairs, like
/// `["pt.x=1", "pt.y=2", "values=1", "values=2", "values=3" "flag"]`
Copy link
Member

Choose a reason for hiding this comment

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

missing comma before flag

/// `["pt.x=1", "pt.y=2", "values=1", "values=2", "values=3" "flag"]`
/// into a structured dictionary.
///
/// - Parameter items: The list of `k=v`-style Strings, each string
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we use the same structure as in the other doc. blocks? Namely a - Paramters:, then newline, then a list of parameters (here - items:).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well when you ask Xcode to generate the doc comment (menu "Editor" > "Structure" > "Add Documentation"), it uses the "- Parameters + one param per line" if there's more than one parameter, but uses directly - Parameter foo if there's only one parameter.

I agree that's inconsistent, and I actually wondered the same as you here. idk if it's better to force consistency ourselves, using - Parameters + one param per line, or to just do as Xcode does (which at least would not force us to fight Xcode and correct every new contributor who are using Xcode's doc auto-generation shortcut)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, leave it then. (I didn't even know that functionality existed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@djbe Approved for merge, then?

Copy link
Member

Choose a reason for hiding this comment

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

Jup!

/// an existing StringDict dictionary being built.
///
/// - Parameters:
/// - parameter: The parameter/string (key/Value pair) to parse
Copy link
Member

Choose a reason for hiding this comment

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

Key or value (case)

/// - Parameters:
/// - parameter: The parameter/string (key/Value pair) to parse
/// - result: The dictionary currently being built and to which to add the value
/// - Returns: The new content of the dictionary being built after nserting the new parsed value
Copy link
Member

Choose a reason for hiding this comment

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

inserting


/// Checks if the string is one of the reserved keywords and if so, escapes it using backticks
///
/// - Parameter in: the string to possibly escape
Copy link
Member

Choose a reason for hiding this comment

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

Use same structure as other doc blocks:

 - Parameters:
   - in: ...


/// This returns the snake cased variant of the string.
///
/// - Parameter string: The string to snake_case
Copy link
Member

Choose a reason for hiding this comment

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

See comments below related to doc block structure.


/// This returns the string with its first parameter uppercased.
/// - note: This is quite similar to `capitalise` except that this filter doesn't
/// lowercase the rest of the string but keep it untouched.
Copy link
Member

Choose a reason for hiding this comment

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

keeps

@djbe djbe merged commit 53cd0c2 into master May 1, 2017
@AliSoftware AliSoftware deleted the feature/split-filters-files branch May 1, 2017 19:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants