Datasources - Part 1: Rework datasource classes #3101
Conversation
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
rolnico
changed the title
|
This is the first part of the division of the PR #2959 |
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
| @Override | ||
| public String getBaseName() { | ||
| return baseName; | ||
| DirectoryDataSource(Path directory, String baseName, |
There was a problem hiding this comment.
add also
DirectoryDataSource(Path directory, String baseName, CompressionFormat compressionFormat) ?
There was a problem hiding this comment.
it wouldn't be used so I'm not sure we should add it.
| * @return true if the file exists, else false | ||
| */ | ||
| @Override | ||
| public boolean exists(String suffix, String ext) throws IOException { |
There was a problem hiding this comment.
move to AbstractFileSystemDatasource and remove the same method from DirectoryDataSource?
…ataSource Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
|
|
||
| UnsupportedOperationException exception = assertThrows(UnsupportedOperationException.class, () -> { | ||
| try (OutputStream ignored = dataSource.newOutputStream("foo.bar", true)) { | ||
| fail(); |
There was a problem hiding this comment.
no need to fail explicitly ?
There was a problem hiding this comment.
It's not needed explicitly but it need something in the try() that cannot throw a runtime exception
|
|
||
| static Stream<Arguments> provideArgumentsForWriteThenReadTest() { | ||
| return Stream.of( | ||
| Arguments.of("foo", "iidm", CompressionFormat.ZIP), |
There was a problem hiding this comment.
should use the same semantics for the "mainExtension" as other test classes
Arguments.of("foo", "iidm", CompressionFormat.ZIP),
vs
Arguments.of("foo", ".iidm", CompressionFormat.ZSTD),
There was a problem hiding this comment.
(or just remove the mainExtension concept from this PR entirely, it's just used for tests and you can put everything in one string)
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
| static Stream<Arguments> provideArgumentsForClassAndListingTest() { | ||
| return Stream.of( | ||
| Arguments.of("foo.iidm", CompressionFormat.BZIP2, Bzip2DirectoryDataSource.class, | ||
| Set.of("foo", "foo.txt", "foo.iidm", "foo.xiidm", "foo.v3.iidm", "foo.v3", "foo_bar.iidm", "foo_bar", |
There was a problem hiding this comment.
maybe factorize the sets as they are the same? which would highlight the fact that so far the listed files are the same for the 3 parameterized tests.
Same comment for other test classes.
| protected Path testDir; | ||
| protected Set<String> unlistedFiles; | ||
| protected Set<String> existingFiles; | ||
| protected ArchiveFormat archiveFormat; |
There was a problem hiding this comment.
this parameter should rather be in AbstractArchiveDataSourceTest
|
|
||
| // Create an archive when needed | ||
| if (archiveFormat != null) { | ||
| createArchiveAndFiles(fileName); |
There was a problem hiding this comment.
This should rather be in the setup of AbstractArchiveDataSourceTest
|
Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
No
What kind of change does this PR introduce?
Feature
What is the current behavior?
All the existing
DataSourcesbased on files extendFileDataSources. The difference between datasources considering files in a directory and the datasources considering files in an archive is not clear enough.What is the new behavior (if this is a feature change)?
Two types of
DataSourcesbased on files now exists:DirectoryDataSource(inherited byGzDirectoryDataSource,Bzip2DirectoryDataSource,ZstdDirectoryDataSource,XZDirectoryDataSource): considers files in a directoryAbstractArchiveDataSource(onlyZipArchiveDataSourcefor now but aTarArchiveDataSourcecould be added later): considers files in an archive.Does this PR introduce a breaking change or deprecate an API?
If yes, please check if the following requirements are fulfilled
What changes might users need to make in their application due to this PR? (migration steps)
Two types of
DataSourcesbased on files now exists:DirectoryDataSource(inherited byGzDirectoryDataSource,Bzip2DirectoryDataSource,ZstdDirectoryDataSource,XZDirectoryDataSource): considers files in a directoryAbstractArchiveDataSource(onlyZipArchiveDataSourcefor now but aTarArchiveDataSourcecould be added later): considers files in an archive.Therefore, multiple classes have changed, please update your code if you used them directly:
Bzip2FileDataSourceBzip2DirectoryDataSourceFileDataSourceDirectoryDataSourceGzFileDataSourceGzDirectoryDataSourceXZFileDataSourceXZDirectoryDataSourceZipFileDataSourceZipArchiveDataSourceZstdFileDataSourceZstdDirectoryDataSourceNote: Their constructors are not the same so you will have to adapt your code to the new ones, depending on which DataSource you use.
Other information: