Skip to content

Conversation

@Lakr233
Copy link
Contributor

@Lakr233 Lakr233 commented Feb 27, 2025

PR for #27

@Lakr233 Lakr233 requested a review from Recouse as a code owner February 27, 2025 03:03
@Recouse
Copy link
Owner

Recouse commented Feb 27, 2025

Would you be able to add tests to verify your solution?

@Lakr233
Copy link
Contributor Author

Lakr233 commented Feb 28, 2025

Hi. I have updated the code to support mixed CRLF and LFLF with tests. But I don't know if we should ever support mixed contents.

@Recouse
Copy link
Owner

Recouse commented Feb 28, 2025

I don't think mixed content will ever be a case.

I'll check the PR and merge it ASAP. Thank you!

Copy link
Owner

@Recouse Recouse left a comment

Choose a reason for hiding this comment

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

Overall, it looks good, but I'm not sure about the replaceSubrange method.

I think we could consider a solution with a custom split method, where we iterate through possible separators, find a range for the separator, split the data, and repeat the process. It will be a split method added in #26, but wrapped in another loop.

To optimize, we could also assume that there won't be mixed separators and use the one from the first iteration.

// now replace CR LF with LF LF for processing mixed content
var data = data
while let crlfRange = data.lastRange(of: [Self.cr, Self.lf]) {
data.replaceSubrange(crlfRange, with: [Self.lf])
Copy link
Owner

Choose a reason for hiding this comment

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

This method has O(n+m) complexity. Won't it affect performance on a large amount of data?

@Lakr233
Copy link
Contributor Author

Lakr233 commented Mar 2, 2025

Would you like to implement the optimization on your side? Kinda busy right now :P

@Recouse
Copy link
Owner

Recouse commented Mar 2, 2025

Sure, I'll give it a shot, and maybe I'll open a new PR

@stallent
Copy link

@Recouse think this PR will come in anytime soon? SSE spec says any of the 3 are supported "Lines must be separated by either a U+000D CARRIAGE RETURN U+000A LINE FEED (CRLF) character pair, a single U+000A LINE FEED (LF) character, or a single U+000D CARRIAGE RETURN (CR) character." so we are choking on spec compliant servers. Thanks! Have had terrific luck with your framework otherwise and appreciate you building it!

@wolfcon
Copy link

wolfcon commented May 13, 2025

Maybe you can change Parser implementation from Data to String.
Use CharacterSet.whitespacesAndNewlines to avoid handle those.

@Recouse
Copy link
Owner

Recouse commented May 13, 2025

I specifically used Data to avoid unnecessary conversions between Data and String. I have some drafts that add CRLF support, I haven't had the time to focus on them full-time, but I'll probably open a new PR as soon as it's ready.

@Lakr233
Copy link
Contributor Author

Lakr233 commented May 13, 2025

Maybe you can change Parser implementation from Data to String.

Use CharacterSet.whitespacesAndNewlines to avoid handle those.

You can't. Some of the data starts with white spaces. It was fixed earlier.

@wolfcon
Copy link

wolfcon commented May 16, 2025

Maybe you can change Parser implementation from Data to String.
Use CharacterSet.whitespacesAndNewlines to avoid handle those.

You can't. Some of the data starts with white spaces. It was fixed earlier.

CharacterSet.newlines ... I have a mistake.

@Lakr233
Copy link
Contributor Author

Lakr233 commented May 27, 2025

@Recouse I have replace all function with O(n) cplx so would you please update with me?

@Recouse
Copy link
Owner

Recouse commented Jul 5, 2025

I've been busy and haven't had time to review it yet. I'll take a look in the coming days, thanks for your contribution.

@Recouse
Copy link
Owner

Recouse commented Jul 6, 2025

Everything looks good to me. There's just one small improvement I'd like to suggest, removing the Data → String → Data conversions in the cleanMessageData(_:) method. I've made the changes and tested them. If you add me as a contributor in your repo, I could push the updates to this PR branch.

@Lakr233
Copy link
Contributor Author

Lakr233 commented Jul 6, 2025

Everything looks good to me. There's just one small improvement I'd like to suggest, removing the Data → String → Data conversions in the cleanMessageData(_:) method. I've made the changes and tested them. If you add me as a contributor in your repo, I could push the updates to this PR branch.

Added.

@Recouse Recouse merged commit 378d44d into Recouse:main Jul 7, 2025
4 checks passed
@Recouse
Copy link
Owner

Recouse commented Jul 7, 2025

Oops, I accidentally pushed everything to my repo instead of yours.

@Recouse Recouse mentioned this pull request Jul 7, 2025
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.

4 participants