Skip to content

Commit e6cfbd0

Browse files
rameelrameel
authored
Merge pull request #65 from rameel/xml-docs-refining
Polish XML comments and fix typos
2 parents c5cb9f4 + 5167a67 commit e6cfbd0

14 files changed

Lines changed: 297 additions & 40 deletions

File tree

‎REVIEW.md‎

Lines changed: 255 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,255 @@
1+
# Code Review: Ramstack.FileSystem
2+
3+
## Обзор
4+
5+
Результаты полного code review библиотеки Ramstack.FileSystem.
6+
Документ служит планом работ по исправлению найденных проблем.
7+
8+
---
9+
10+
## 1. Баги и проблемы в коде
11+
12+
### 1.1 Отсутствие проверки отмены в цикле `WriteAllLinesAsync`
13+
14+
**Файл:** `src/Ramstack.FileSystem.Abstractions/VirtualFileExtensions.cs`, строки 336–337
15+
**Серьёзность:** Средняя
16+
17+
Метод `WriteAllLinesAsync` принимает `CancellationToken`, но не проверяет отмену внутри цикла `foreach`. Это несовместимо с `ReadAllLinesAsync` (строка 147), где `cancellationToken.ThrowIfCancellationRequested()` вызывается корректно.
18+
19+
**Сейчас:**
20+
```csharp
21+
foreach (var line in contents)
22+
await writer.WriteLineAsync(line).ConfigureAwait(false);
23+
```
24+
25+
**Исправление:**
26+
```csharp
27+
foreach (var line in contents)
28+
{
29+
cancellationToken.ThrowIfCancellationRequested();
30+
await writer.WriteLineAsync(line).ConfigureAwait(false);
31+
}
32+
```
33+
34+
---
35+
36+
### 1.2 Некорректная обработка отмены в `AsyncEnumeratorAdapter`
37+
38+
**Файл:** `src/Ramstack.FileSystem.Abstractions/Utilities/EnumerableExtensions.cs`, строка 51
39+
**Серьёзность:** Средняя
40+
41+
`MoveNextAsync` при отмене возвращает `false` вместо выброса `OperationCanceledException`. Это маскирует отмену под нормальное завершение перечисления.
42+
43+
**Сейчас:**
44+
```csharp
45+
var result = !cancellationToken.IsCancellationRequested && enumerator.MoveNext();
46+
return new ValueTask<bool>(result);
47+
```
48+
49+
**Исправление:**
50+
```csharp
51+
cancellationToken.ThrowIfCancellationRequested();
52+
return new ValueTask<bool>(enumerator.MoveNext());
53+
```
54+
55+
---
56+
57+
### 1.3 Отсутствие проверки `null` в конструкторе `CompositeFileSystem`
58+
59+
**Файл:** `src/Ramstack.FileSystem.Composite/CompositeFileSystem.cs`, строки 36–37
60+
**Серьёзность:** Средняя
61+
62+
Конструктор, принимающий `IEnumerable<IVirtualFileSystem>`, не проверяет аргумент на `null`, в отличие от `params`-перегрузки (строка 28).
63+
64+
**Сейчас:**
65+
```csharp
66+
public CompositeFileSystem(IEnumerable<IVirtualFileSystem> fileSystems) =>
67+
InternalFileSystems = fileSystems.ToArray();
68+
```
69+
70+
**Исправление:**
71+
```csharp
72+
public CompositeFileSystem(IEnumerable<IVirtualFileSystem> fileSystems)
73+
{
74+
ArgumentNullException.ThrowIfNull(fileSystems);
75+
InternalFileSystems = fileSystems.ToArray();
76+
}
77+
```
78+
79+
---
80+
81+
### 1.4 Отсутствие вызова `base.Dispose(disposing)` в `S3UploadStream`
82+
83+
**Файл:** `src/Ramstack.FileSystem.Amazon/S3UploadStream.cs`, строки 153–160
84+
**Серьёзность:** Низкая (для `Stream` — no-op, но нарушение паттерна)
85+
86+
Метод `Dispose(bool)` делегирует работу `DisposeAsync()`, но не вызывает `base.Dispose(disposing)`.
87+
88+
**Исправление:**
89+
```csharp
90+
protected override void Dispose(bool disposing)
91+
{
92+
if (disposing)
93+
{
94+
using var scope = NullSynchronizationContext.CreateScope();
95+
DisposeAsync().AsTask().Wait();
96+
}
97+
98+
base.Dispose(disposing);
99+
}
100+
```
101+
102+
---
103+
104+
## 2. Опечатки в XML-комментариях
105+
106+
### 2.1 Дублирование артикля "the"
107+
108+
**Файл:** `src/Ramstack.FileSystem.Abstractions/VirtualFileSystemExtensions.cs`, строка 192
109+
110+
**Сейчас:** `"Asynchronously writes the specified string to the specified the file."`
111+
**Исправление:** `"Asynchronously writes the specified string to the specified file."`
112+
113+
---
114+
115+
### 2.2 Перепутан порядок слов
116+
117+
**Файл:** `src/Ramstack.FileSystem.Physical/PhysicalFileSystem.cs`, строка 24
118+
119+
**Сейчас:** `"The physical path of root the directory."`
120+
**Исправление:** `"The physical path of the root directory."`
121+
122+
---
123+
124+
## 3. Несогласованности в XML-комментариях
125+
126+
### 3.1 Описание параметра `cancellationToken`
127+
128+
Основной вариант (подавляющее большинство): `"An optional cancellation token to cancel the operation."`
129+
130+
Нестандартные варианты, требующие унификации:
131+
132+
| Файл | Строка | Текущий текст |
133+
|------|--------|--------------|
134+
| `VirtualFileSystemExtensions.cs` | 27 | `"The optional cancellation token used for canceling the operation."` |
135+
| `VirtualFileSystemExtensions.cs` | 40 | `"The optional cancellation token used for canceling the operation."` |
136+
| `VirtualFileSystemExtensions.cs` | 276 | `"A cancellation token to cancel the operation."` |
137+
| `VirtualFileSystemExtensions.cs` | 302 | `"An optional cancellation token to cancel the operation. Defaults to <see cref="CancellationToken.None"/>."` |
138+
| `VirtualFileSystemExtensions.cs` | 316 | `"A token to cancel the operation. Defaults to <see cref="CancellationToken.None"/>."` |
139+
| `VirtualFileSystemExtensions.cs` | 335 | `"A cancellation token to cancel the operation."` |
140+
| `VirtualFileExtensions.cs` | 384 | `"An optional cancellation token to cancel the operation. Defaults to <see cref="CancellationToken.None"/>."` |
141+
| `VirtualNode.cs` | 65 | `"A cancellation token to cancel the operation."` |
142+
| `VirtualNode.cs` | 120 | `"A cancellation token to cancel the operation."` |
143+
| `GcsFile.cs` | 145 | `"A cancellation token to cancel the operation."` |
144+
| `AzureFile.cs` | 120 | `"A cancellation token to cancel the operation."` |
145+
| `S3File.cs` | 137 | `"A cancellation token to cancel the operation."` |
146+
| `S3UploadStream.cs` | 218 | `"A cancellation token to cancel the operation."` |
147+
| `S3UploadStream.cs` | 272 | `"A cancellation token to cancel the operation."` |
148+
| `AmazonS3FileSystem.cs` | 111 | `"A cancellation token to cancel the operation."` |
149+
| `AmazonS3FileSystem.cs` | 122 | `"A cancellation token to cancel the operation."` |
150+
151+
**Рекомендация:** Унифицировать все до `"An optional cancellation token to cancel the operation."` — без суффикса `"Defaults to..."`.
152+
153+
---
154+
155+
### 3.2 Описание параметра `encoding`
156+
157+
Три разных варианта:
158+
159+
| Текст | Файлы и строки |
160+
|-------|----------------|
161+
| `"The character encoding to use."` | `VirtualFileExtensions.cs:29`, `VirtualFileSystemExtensions.cs:39` |
162+
| `"The encoding applied to the contents."` | `VirtualFileExtensions.cs:69,133`, `VirtualFileSystemExtensions.cs:115,142` |
163+
| `"The encoding to apply to the string."` | `VirtualFileExtensions.cs:272,297,326`, `VirtualFileSystemExtensions.cs:183,210,237` |
164+
165+
**Рекомендация:** Унифицировать до `"The encoding to use."` или разграничить: для чтения — `"The encoding applied to the contents."`, для записи — `"The encoding to apply to the string."`.
166+
167+
---
168+
169+
### 3.3 Использование "should" вместо "will" в remarks
170+
171+
**Файл:** `src/Ramstack.FileSystem.Abstractions/VirtualFile.cs`, строки 188–190
172+
173+
В `VirtualFile.WriteCoreAsync` remarks используют `"should"`:
174+
```xml
175+
<item><description>If the file does not exist, it should be created.</description></item>
176+
<item><description>...the existing file should be overwritten.</description></item>
177+
<item><description>...an exception should be thrown.</description></item>
178+
```
179+
180+
В `VirtualFileSystemExtensions.cs` (строки 322–324) аналогичный remarks использует `"will"`:
181+
```xml
182+
<item><description>If the file does not exist, it will be created.</description></item>
183+
```
184+
185+
**Рекомендация:** Для абстрактного метода (`WriteCoreAsync`) `"should"` может быть допустимо (предписание реализации), но для единообразия лучше использовать `"must"` или `"will"`.
186+
187+
---
188+
189+
### 3.4 Несогласованность `<returns>` для async-методов
190+
191+
**Файл:** `src/Ramstack.FileSystem.Abstractions/VirtualFileSystemExtensions.cs`, строки 29, 42
192+
193+
Используется `"A task representing the asynchronous operation and returns..."` — это не стандартный формат и не упоминает `ValueTask`/`Task` через `<see cref="..."/>`.
194+
195+
Остальные методы используют:
196+
```xml
197+
/// A <see cref="ValueTask"/> representing the asynchronous operation.
198+
```
199+
200+
**Рекомендация:** Унифицировать до:
201+
```xml
202+
/// A <see cref="Task{TResult}"/> representing the asynchronous operation.
203+
/// The task result contains a <see cref="StreamReader"/> that reads from the text file.
204+
```
205+
206+
---
207+
208+
### 3.5 Описание параметра `file` в обёрточных классах
209+
210+
Разные формулировки для одного и того же:
211+
212+
| Файл | Текст |
213+
|------|-------|
214+
| `CompositeFile.cs:21` | `"The <see cref="VirtualFile"/> to wrap."` |
215+
| `GlobbingFile.cs:21` | `"The <see cref="VirtualFile"/> instance to wrap."` |
216+
| `ReadonlyFile.cs:18` | `"The <see cref="VirtualFile"/> instance to wrap."` |
217+
| `PrefixedFile.cs:20` | `"The underlying <see cref="VirtualFile"/> that this instance wraps."` |
218+
| `SubFile.cs:19` | `"The underlying <see cref="VirtualFile"/> instance to wrap."` |
219+
220+
**Рекомендация:** Унифицировать, например, до `"The <see cref="VirtualFile"/> instance to wrap."`.
221+
222+
---
223+
224+
### 3.6 Описание параметра `fs` в `VirtualFileSystemExtensions.cs`
225+
226+
Два варианта:
227+
228+
| Текст | Строки |
229+
|-------|--------|
230+
| `"The file system to use."` | Большинство методов |
231+
| `"The <see cref="IVirtualFileSystem"/> instance."` | Строки 299, 312 (CopyFileAsync) |
232+
233+
**Рекомендация:** Унифицировать до `"The file system to use."`.
234+
235+
---
236+
237+
## 4. План работ
238+
239+
### Приоритет: Высокий
240+
1. Исправить опечатку "the specified the file" (`VirtualFileSystemExtensions.cs:192`)
241+
2. Исправить опечатку "root the directory" (`PhysicalFileSystem.cs:24`)
242+
3. Добавить `cancellationToken.ThrowIfCancellationRequested()` в `WriteAllLinesAsync` (`VirtualFileExtensions.cs:336`)
243+
4. Исправить `AsyncEnumeratorAdapter.MoveNextAsync` — бросать `OperationCanceledException` вместо возврата `false` (`EnumerableExtensions.cs:51`)
244+
5. Добавить `ArgumentNullException.ThrowIfNull` в конструктор `CompositeFileSystem` (`CompositeFileSystem.cs:36`)
245+
246+
### Приоритет: Средний
247+
6. Унифицировать описания `cancellationToken` (16 мест — см. раздел 3.1)
248+
7. Унифицировать описания `encoding` (12 мест — см. раздел 3.2)
249+
8. Исправить "should" → "will"/"must" в `VirtualFile.cs:188-190`
250+
9. Унифицировать `<returns>` для `OpenTextAsync` (`VirtualFileSystemExtensions.cs:29,42`)
251+
10. Унифицировать описания параметра `file` в обёрточных классах (5 мест — см. раздел 3.5)
252+
11. Унифицировать описания параметра `fs` (`VirtualFileSystemExtensions.cs:299,312`)
253+
254+
### Приоритет: Низкий
255+
12. Добавить `base.Dispose(disposing)` в `S3UploadStream.Dispose` (`S3UploadStream.cs:153`)

‎src/Ramstack.FileSystem.Abstractions/VirtualFile.cs‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ public ValueTask<Stream> OpenWriteAsync(CancellationToken cancellationToken = de
7272
/// <remarks>
7373
/// <list type="bullet">
7474
/// <item><description>If the file does not exist, it will be created.</description></item>
75-
/// <item><description>If it exists and <paramref name="overwrite"/> is <see langword="true"/>, the existing file will be overwritten.</description></item>
75+
/// <item><description>If the file exists and <paramref name="overwrite"/> is <see langword="true"/>, the existing file will be overwritten.</description></item>
7676
/// <item><description>If <paramref name="overwrite"/> is <see langword="false"/> and the file exists, an exception will be thrown.</description></item>
7777
/// </list>
7878
/// </remarks>
@@ -111,7 +111,7 @@ public ValueTask DeleteAsync(CancellationToken cancellationToken = default)
111111
/// <remarks>
112112
/// <list type="bullet">
113113
/// <item><description>If the file does not exist, it will be created.</description></item>
114-
/// <item><description>If it exists and <paramref name="overwrite"/> is <see langword="true"/>, the existing file will be overwritten.</description></item>
114+
/// <item><description>If the file exists and <paramref name="overwrite"/> is <see langword="true"/>, the existing file will be overwritten.</description></item>
115115
/// <item><description>If <paramref name="overwrite"/> is <see langword="false"/> and the file exists, an exception will be thrown.</description></item>
116116
/// </list>
117117
/// </remarks>
@@ -137,7 +137,7 @@ public ValueTask CopyToAsync(string destinationPath, bool overwrite, Cancellatio
137137
/// <remarks>
138138
/// <list type="bullet">
139139
/// <item><description>If the file does not exist, it will be created.</description></item>
140-
/// <item><description>If it exists and <paramref name="overwrite"/> is <see langword="true"/>, the existing file will be overwritten.</description></item>
140+
/// <item><description>If the file exists and <paramref name="overwrite"/> is <see langword="true"/>, the existing file will be overwritten.</description></item>
141141
/// <item><description>If <paramref name="overwrite"/> is <see langword="false"/> and the file exists, an exception will be thrown.</description></item>
142142
/// </list>
143143
/// </remarks>
@@ -185,9 +185,9 @@ public ValueTask CopyToAsync(VirtualFile destination, bool overwrite, Cancellati
185185
/// </returns>
186186
/// <remarks>
187187
/// <list type="bullet">
188-
/// <item><description>If the file does not exist, it should be created.</description></item>
189-
/// <item><description>If the file exists and <paramref name="overwrite"/> is <see langword="true"/>, the existing file should be overwritten.</description></item>
190-
/// <item><description>If <paramref name="overwrite"/> is <see langword="false"/> and the file exists, an exception should be thrown.</description></item>
188+
/// <item><description>If the file does not exist, it must be created.</description></item>
189+
/// <item><description>If the file exists and <paramref name="overwrite"/> is <see langword="true"/>, the existing file must be overwritten.</description></item>
190+
/// <item><description>If <paramref name="overwrite"/> is <see langword="false"/> and the file exists, an exception must be thrown.</description></item>
191191
/// </list>
192192
/// </remarks>
193193
protected abstract ValueTask WriteCoreAsync(Stream stream, bool overwrite, CancellationToken cancellationToken);
@@ -212,9 +212,9 @@ public ValueTask CopyToAsync(VirtualFile destination, bool overwrite, Cancellati
212212
/// </returns>
213213
/// <remarks>
214214
/// <list type="bullet">
215-
/// <item><description>If the file does not exist, it will be created.</description></item>
216-
/// <item><description>If it exists and <paramref name="overwrite"/> is <see langword="true"/>, the existing file will be overwritten.</description></item>
217-
/// <item><description>If <paramref name="overwrite"/> is <see langword="false"/> and the file exists, an exception will be thrown.</description></item>
215+
/// <item><description>If the file does not exist, it must be created.</description></item>
216+
/// <item><description>If the file exists and <paramref name="overwrite"/> is <see langword="true"/>, the existing file must be overwritten.</description></item>
217+
/// <item><description>If <paramref name="overwrite"/> is <see langword="false"/> and the file exists, an exception must be thrown.</description></item>
218218
/// </list>
219219
/// </remarks>
220220
protected virtual async ValueTask CopyToCoreAsync(string destinationPath, bool overwrite, CancellationToken cancellationToken)
@@ -234,9 +234,9 @@ protected virtual async ValueTask CopyToCoreAsync(string destinationPath, bool o
234234
/// </returns>
235235
/// <remarks>
236236
/// <list type="bullet">
237-
/// <item><description>If the file does not exist, it will be created.</description></item>
238-
/// <item><description>If it exists and <paramref name="overwrite"/> is <see langword="true"/>, the existing file will be overwritten.</description></item>
239-
/// <item><description>If <paramref name="overwrite"/> is <see langword="false"/> and the file exists, an exception will be thrown.</description></item>
237+
/// <item><description>If the file does not exist, it must be created.</description></item>
238+
/// <item><description>If the file exists and <paramref name="overwrite"/> is <see langword="true"/>, the existing file must be overwritten.</description></item>
239+
/// <item><description>If <paramref name="overwrite"/> is <see langword="false"/> and the file exists, an exception must be thrown.</description></item>
240240
/// </list>
241241
/// </remarks>
242242
protected virtual async ValueTask CopyToCoreAsync(VirtualFile destination, bool overwrite, CancellationToken cancellationToken)

‎src/Ramstack.FileSystem.Abstractions/VirtualFileExtensions.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ public static ValueTask<StreamReader> OpenTextAsync(this VirtualFile file, Cance
2626
/// that reads from the specified text file.
2727
/// </summary>
2828
/// <param name="file">The file to get the <see cref="StreamReader"/> for.</param>
29-
/// <param name="encoding">The character encoding to use.</param>
29+
/// <param name="encoding">The encoding applied to the contents.</param>
3030
/// <param name="cancellationToken">An optional cancellation token to cancel the operation.</param>
3131
/// <returns>
3232
/// A <see cref="ValueTask{TResult}"/> representing the asynchronous operation.

0 commit comments

Comments
 (0)