Introduce two new docs covering codecs and converters#7211
Conversation
|
Visit the preview URL for this PR (updated for commit d833eff): https://dart-dev--pr7211-feat-converters-and-codecs-6xdboazt.web.app |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
parlough
changed the title
|
@lrhn would you be able to take a look at these two pages covering converters and codecs? I'd appreciate any insights you have! @conooi When you have a chance, this PR is ready for an initial look. @lamek This PR includes a variety of code excerpts if you'd like to take a look. Thanks all! I appreciate it :) |
lrhn
left a comment
lrhn
left a comment
There was a problem hiding this comment.
LGTM! Super update.
I tend to want to over-communicate all the details, so take my suggestions with an unhealthy amount of salt.
| /// Shifts the specified [codeUnit] by | ||
| /// [shift] positions in the alphabet. | ||
| /// | ||
| /// Only shifts lowercase ASCII letters (a-z). |
There was a problem hiding this comment.
(Can farily easily do upper case too, just create
const a = 0x61;
const z = 0x7a;
const caseDelta = 0x20;
final lowerCodeUnit = codeUnit | caseDelta;
if (lowerCodeUnit >= a && lowerCodeUnit <= z) {
final letterNumber = lowerCodeUnit - a;
return codeUnit - letterNumber + (letterNumber + shift).remainder(26);
}Easier than having to explain why it doesn't do it.)
| void addSlice(String chunk, int start, int end, bool isLast) { | ||
| final buffer = StringBuffer(); | ||
| for (var i = start; i < end; i++) { | ||
| buffer.writeCharCode(_shiftCodeUnit(chunk.codeUnitAt(i), _shift)); |
There was a problem hiding this comment.
(I'd have a String _shiftCodeUnits(String input, int shift) {...} helper. You write this code repeatedly. Well, at least twice.)
| const a = 0x61; | ||
| const z = 0x7A; | ||
| if (codeUnit >= a && codeUnit <= z) { | ||
| return a + (codeUnit - a + shift) % 26; |
There was a problem hiding this comment.
Use .remainder(26) instead of % 26.
Using .remainder(26) guarantees a result in the range 0..25.
There is no guard on the shift value, so it can potentially be negative, which can make (codeUnit - a + shift) negative and then %26 gives a negative result.
There was a problem hiding this comment.
I'm coming back to this a few months later (thanks for the thorough review), and I wanted to confirm this suggestion.
https://api.dart.dev/dart-core/num/operator_modulo.html suggests for the % operator, "The sign of the returned value r is always positive". Doesn't that match what we want here? A bit of a contrast compared to many other languages like Java and JS though, so perhaps it's worth me explicitly calling out the behavior of %?
| @@ -0,0 +1,172 @@ | |||
| import 'dart:convert'; | |||
|
|
|||
| // #docregion caesar-encoder-sink | |||
There was a problem hiding this comment.
(For readability of the file itself, I'd put the CaesarCodec first, then CaesarConverter, and then the private classes they depend on. But that's just, like, my way of structuring things: Most important first, things it depends on below. If you don't need to know the details of the dependencies, you can just not scroll down, and if you need to find a detail, you only need to look below, not above.
If this file only exists as a file to extract chunks from for the article, then its readability probably isn't important.)
|
|
||
| // #docregion caesar-encoder-sink | ||
| /// A [StringConversionSink] that applies a Caesar cipher [_shift] and | ||
| /// forwards the result to the [_output] sink. |
There was a problem hiding this comment.
Single-line first paragraph.
Consider:
/// A [StringConversionSink] that applies a Casar cipher.
///
/// Shifts inputs by [_shift] and emits the shifted strings on
/// [_output].| // Read a file as a stream of bytes, decode UTF-8 into a string, | ||
| // then split the single string into lines. | ||
| final lines = File('data.txt') | ||
| .openRead() // |
There was a problem hiding this comment.
Why // here? Trying to coerce the formatter?
| // #docregion stream-lines | ||
| void streamLinesExample() async { | ||
| // Read a file as a stream of bytes, decode UTF-8 into a string, | ||
| // then split the single string into lines. |
There was a problem hiding this comment.
Not really a single string. That would be the description of:
final bytes = File('data.txt').readAsBytesSync();
final string = utf8.decode(bytes);
final lines = const LineSplitter().convert(string);The streaming version specifically doesn't create a single string.
|
|
||
| // #docregion encoding-lookup | ||
| void encodingLookupExample() { | ||
| final encoding = Encoding.getByName('utf-8'); |
There was a problem hiding this comment.
I'd put a ! after the lookup instead of a ?. in the use below.
| `Encoding` also provides a default implementation of [`decodeStream`][]. | ||
| In contrast to `Stream.transform` which returns a `Stream<String>`, | ||
| the `decodeStream` method reads a byte stream and | ||
| returns a single decoded `String` (wrapped in a `Future`). |
There was a problem hiding this comment.
returns ... (wrapped in a Future) -> asynchronously returns ...
Or just -> return ...
People know that "returns Future<String>" means "returns String, but later".
| If the conversion is about how data is _represented_, | ||
| such as with bytes, strings, or compressed data, | ||
| the codec pattern is a natural fit. | ||
| If the conversion is about how data is _structured_, |
There was a problem hiding this comment.
(Well, that is a very good description of what I tried to say above. Say the same thing there!)
4 participants
Inspired by the pre-existing Converters and codecs page, introduce two new articles that are more focused than the original and follow our current standards for writing and structure.
Resolves #5175