TheBoxyBear / charttools

Library for editing Clone Hero song files in .NET
https://theboxybear.github.io/charttools
GNU General Public License v3.0
13 stars 4 forks source link

Improper use of `WritingDataSource` #84

Closed TheBoxyBear closed 1 week ago

TheBoxyBear commented 1 week ago

Severity: Critical

Replace methods in ChartFile are meant to replace components while retaining the sections of other components from the data source. However, WritingDataSource requires an additional parameter to define existing data to maintain, while Replace methods only provide one source to write to, leading to existing data being discarded.

Replace methods should have an overload taking an existing ReadingDataSource to pass to the WritingDataSource. If the original methods are used with a path or stream, the same parameter is used as existing data. To avoid an overabundance or overloads, DataSource types may also define implicit conversions from string and stream, allowing such parameters to be passed in place of a source without relying on overloads. This would also eliminate overloads for reading.

TheBoxyBear commented 1 week ago

This should only applies to the beta branch following the backend reword for stream support.

TheBoxyBear commented 1 week ago

Implicit conversions are now supported with proper disposal based on the lifetime of the reader/writer. However, only receiving a data source that may be implicitly converted prevents the adding or removing an existing reading source from an external parameter.

Should implicit conversions include the write source as an existing read source? This would work for most methods based on their prefix Replace but not for WriteSong. The ReplaceSong case could be remedied by having overloads which would take precedence over implicit conversions.