Fix 699 unsafe filenames

[SDGGiesbrecht]

This would fix #699, #146, #361, #547 and #558 in one fell swoop, but probably needs thorough review.

See here for the rationale.
See here for a quick way to test it.

@jpsim, what are your thoughts?

[SDGGiesbrecht]

“Catch URL‐illegal characters in filenames.” Someday I will learn to spell... ☺

[jpsim]

This is awesome! I really appreciate the time you took to carefully consider the discussions that took place in previous issues and PRs when doing this. Looks like there are a few stylistic issues that rubocop caught. You can validate these yourself by running bundle install followed by bundle exec rake rubocop. Since this option is off by default, the integration specs don't need to be updated, so that's easy. Ping me once this is passing CI and I'll do more of a thorough review, but since this is opt-in behavior, I doubt it'll be contentious.

[SDGGiesbrecht]

Thanks for the tip. I wasn’t sure how to run rubocop myself, and the path of least resistance so far was to wait for your CI suite to run it for me. ☺

[SDGGiesbrecht]

It satisfies RuboCop now.

One thing that has since occurred to me is that the mapping is many‐to‐one. i.e. /(_:_:) and _2F(_:_:) will both wind up as _2F_28__3A__3A_29, and the two functions would get clashing filenames.

I don’t think this would ever be a real‐world issue, because any API designed with underscores and hexadecimal needs to be rethought. Also the leading underscore is normally a mark of an unwillingly public symbol, which usually ought to receive /// :nodoc: and never survive far enough into the program to be assigned a filename.

Nevertheless, if anyone thinks this is a real concern, the fix is simply to extend the line
CGI.escape(normalized).tr('%', '_')
with a step that manually encodes any real underscores
CGI.escape(normalized).tr('_', '%5F').tr('%', '_')

@jpsim do you think this should be done?

[jpsim]

do you think this should be done?

Yes, might as well make the safe URLs option generate truly safe URLs 😉

[SDGGiesbrecht]

Will do.

[SDGGiesbrecht]

Don’t merge yet. I want to test something first.

[SDGGiesbrecht]

I should have read the documentation for tr before I naïvely switched it in for gsub like RuboCop suggested.

CGI.escape(normalized).tr('_', '%5F').tr('%', '_') is not at all what I wanted.
CGI.escape(normalized).gsub('_', '%5F').tr('%', '_') does what I intended.

By now it should be apparent that Ruby isn’t my mother tongue...

It’s fixed now though and all is well.

[SDGGiesbrecht]

@jpsim, I’m considering writing some minimalist test cases for realm/jazzy-integration-specs to make sure these last few bugs don’t return (two of them didn’t affect the existing tests), but I’m not entirely sure I understand its set‐up.

If I just add to the contents of misc_jazzy_features/before, run bundle exec rake rebuild_integration_fixtures and submit a pull request, will all be well, or is additional configuration required?

[jpsim]

That sounds right. To make sure we're clear (sorry if redundant):

1. Specify use_safe_filenames in the misc_jazzy_features spec's .jazzy.yaml file: https://github.com/realm/jazzy-integration-specs/blob/master/misc_jazzy_features/before/.jazzy.yaml
2. Run bundle exec rake rebuild_integration_fixtures from the root of this repo, making sure that Xcode 7.3.1 is installed and xcode-selected.
3. Navigate to spec/integration_specs to inspect the diff to make sure your change is working as expected, and that there's weren't other issues that creeped in while rebuilding the specs.
4. Make a PR towards realm/jazzy-integration-specs with the changes
5. Once that's merged to that repo's master branch, update this PR to point to the tip of master.

[SDGGiesbrecht]

Thanks. I’ll give it a shot.

[SDGGiesbrecht]

I can’t seem to get the set‐up correct. No matter how I arrange my local repositories, bundle exec rake rebuild_integration_fixtures just prints the following and hangs.

--------------------------------------------------------------------------------
Running Integration tests
--------------------------------------------------------------------------------

rm -rf spec/integration_specs/tmp

This has turned into a lot of work for just a unit test when the bugs have already been fixed. I don’t think I want to fight with it any further. If someone else wants to take charge (especially someone not needing to juggle forks), the following would just need to go into one of the files in realm/jazzy-integration-specs/misc_jazzy_features/before/MiscJazzyFeatures/ to guard against a regression.

/// Function with characters unsafe for filenames
public func /<T>(lhs: T, rhs: T) {}

(along with use_safe_filenames: true in realm/jazzy-integration-specs/misc_jazzy_features/before/.jazzy.yaml)

And the following (in the same place) would guard against a regression of #673:

/// Function (recommended documentation comment style)
///
/// - Parameters:
///     - aParameter: A parameter
///
/// - Returns: A result
public func functionA(aParameter: Bool) -> Bool {
    return true
}

/// Function (alternate documentation comment style)
///
/// - Parameter: aParameter: A parameter
public func functionB(aParameter: Bool) -> Bool {
    return true
}

/// Function (non‐standard documentation comment style)
///
/// - parameters:
///     - aParameter: A parameter
///
/// - returns: A result
public func functionC(aParameter: Bool) -> Bool {
    return true
}

/// Function (alternate non‐standard documentation comment style)
///
/// - parameter: aParameter: A parameter
public func functionD(aParameter: Bool) -> Bool {
    return true
}

[SDGGiesbrecht]

One final note: I did notice I forgot to update the example in the changelog to match the minor change we made to the algorithm. I will do that right away.

[SDGGiesbrecht]

Unless the integration tests fail, I think everything is ready from my end.

[jpsim]

I'm sorry to hear you've been having trouble updating the integration specs. Are you sure you've recursively cloned the git submodules? git submodule update --init --recursive

I should be able to take this from here... I'll let you know if I run into any problems...

[jpsim]

@SDGGiesbrecht here's the integration specs diff: realm/jazzy-integration-specs@5abc501

I've noticed that some of the links are now broken due to this change. Like clicking on the sidebar categories (e.g. "Other Classes") links to a version of the URL with spaces (Other Classes.html) rather than the plusses (Other+Classes.html).

I'm quite impressed with your level of contributions so far, so I've given you write access to both this repo and the integration specs one. Hopefully that will make it a bit easier for you to contribute, especially without having to juggle different forks and submodules pointing to different remotes 😅.

Let me know if you'd like more help with this or if you think you have it covered..

[SDGGiesbrecht]

Thanks. I’ll have a look.

[SDGGiesbrecht]

Continuing in #701.

[SDGGiesbrecht]

Continuing in #702.