Skip to content
This repository has been archived by the owner on Oct 17, 2024. It is now read-only.

Add chunked decoding support to CodePage #91

Merged
merged 5 commits into from
Nov 6, 2023

Conversation

brianquinlan
Copy link
Contributor

@brianquinlan brianquinlan commented Nov 3, 2023

  • Add chunked decoding support (startChunkedConversion) for CodePage encodings. This is straightforward because CodePage encodings map a single byte to set of code units so state must be preserved between chunks.

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

Add chunked decoding support (`startChunkedConversion`) for `CodePage` encodings
lib/src/codepage.dart Outdated Show resolved Hide resolved
lib/src/codepage.dart Outdated Show resolved Hide resolved
lib/src/codepage.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@lrhn lrhn left a comment

Choose a reason for hiding this comment

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

LGTM. And good tests!

lib/src/codepage.dart Outdated Show resolved Hide resolved
lib/src/codepage.dart Outdated Show resolved Hide resolved
});

test('chunked conversion', () {
late final String decodedString;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just initialize it to "" instead of being late. (I don't like late 😉 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like mutability ;-) I'd prefer to have late and make it final rather lose the final specifier ;-)

group('Custom code page', () {
late final CodePage cp;

setUpAll(() => cp = CodePage('custom', "ABCDEF${"\uFFFD" * 250}"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Code pages should be immutable, so just initialize it directly, without using setUp. (I also don't like setUp 😁).

Unless you fear that it will throw?

(What's the difference between setUp and setUpAll?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setUp runs for each embedded test, setUpAll runs once.

Done.

@brianquinlan
Copy link
Contributor Author

FYI, I changed the minimum supported SDK version to 3.0 to get the addSlice implementation from ByteConversionSink

@brianquinlan brianquinlan merged commit 3503170 into dart-archive:master Nov 6, 2023
4 checks passed
mosuem pushed a commit to dart-lang/core that referenced this pull request Oct 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants