Filter: Only remove multiple spaces

[djbe]

Modify the removeNewlines filter to only remove whitespace characters:

• if they're a newline
• if there are multiple consecutive whitespace chars
• if they're at either end of the string (trimming)

Remaining single whitespace characters are converted to a space .

[AliSoftware]

Not sure why the beginning & end whitespaces would be kept there? I'd expect them to be removed by .trimmingCharacters(in: .whitespacesAndNewlines) here.

[djbe]

This is for the false case, where only newlines are removed.This is

[AliSoftware]

Oh, right.

[AliSoftware]

Maybe add that " test test ":"test test" here to confirm my suggestion about that it should remove the leading/trailing whitespaces in the examples you put in the md docs above

[AliSoftware]

Still, I like that the examples we give in the MD documentation are used in the tests as well, if only to be sure that we are consistent with what we state in the doc and that those cases are indeed checked

[djbe]

I'll add that specific case, but I'll number it (easier for debugging if needed) like the rest.

[djbe]

Hmmm, instead of doing multiple regexes, would it be easier (for the removewhitespaces case) to:

• split based on newlines
• trim each line
• join again

[AliSoftware]

As long as the test cases still passes when you change the implementation… 😉
I'm not sure what would be faster, but RegExes even if powerful are also generally slower indeed, so might be worth it yeah

[djbe]

I'm just wondering what would be useful for users (and our use cases).

The main one will be the ability to write things like macros on multiple lines but still have the result be one line. This might mean multiple things though:

• I don't want any spaces at all, anywhere
• Just remove the newlines and leading whitespaces
• ...?

[AliSoftware]

That's pretty much the use cases I see indeed. Creating some identifier or word from a complex expression (= no spaces at all anywhere) or formatting the template nicely (remove newlines and leading spaces per lines), so I think this covers all we need for now?

[djbe]

Hmmm no then I need to change the code a bit. Right now it either:

• only removes newlines and nothing else
• removes newlines and multiple consecutive spaces + replaces single whitespaces with a space

[djbe]

Instead of a boolean parameter, should we maybe accept a string to represent a bunch of cases ("all", "leading", ...)?

[AliSoftware]

Mmmh I was gonna say YAGNI, but given that the different cases are not that straightforward to explain ("remove whitespaces except this but that…"), a string instead of a bool might be more explicit anyway, so yeah, go for it.

[AliSoftware]

Shouldn't we throw if the argument is not a valid string, like removeNewlines:42?

So if there's no argument at all (arguments.first == nil), agreed that all should be the default. But if the argument isn't castable as a as? String, maybe throw?

[AliSoftware]

Use some constants instead of magic strings here? Even if those constants are scoped inside the func.

e.g.

static func removeNewlines(_ value: Any? arguments: [Any?]) throws -> Any? {
  enum Modes: String {
    case all, leading
  }

  let modeArg = String(describing: arguments.first) ?? Modes.all.rawValue
  guard let mode = Modes(rawValue: modeArg) else {
    throw Filters.Error.invalidOption(option: modeArg)
  }

  switch mode {
  case .all: …
  case .leading: …
  }
}

[djbe]

Before you ask, the reason that enum is not inside the function is because swiftlint complains that such things can be at most 1 level deep (nesting) 🤷‍♂️

[AliSoftware]

Codebeat seems to be unhappy with nesting too deep here, maybe separate the closure used in that map in a private function to make it happy?

[AliSoftware]

Add a test with a non-zero index argument too