Introducing SqlIndividualTimeSeriesMetaInformation & refactorings #518
Conversation
sebastian-peter
force-pushed
the
sp/#513-SqlIndividualTimeSeriesMetaInformation
branch
from
bb5d671 to
4963ad2
Compare
sebastian-peter
force-pushed
the
sp/#513-SqlIndividualTimeSeriesMetaInformation
branch
from
4963ad2 to
b6b6600
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## dev #518 +/- ##
=========================================
Coverage 78.94% 78.95%
+ Complexity 2131 2127 -4
=========================================
Files 265 265
Lines 8356 8371 +15
Branches 785 788 +3
=========================================
+ Hits 6597 6609 +12
- Misses 1356 1362 +6
+ Partials 403 400 -3
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
src/main/java/edu/ie3/datamodel/io/connectors/SqlConnector.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/ie3/datamodel/io/source/sql/SqlTimeSeriesMappingSource.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/ie3/datamodel/io/source/sql/SqlTimeSeriesMappingSource.java
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| /** Meta information, that can be derived from a certain file name */ | ||
| public abstract class FileNameMetaInformation { | ||
| /** Meta information, that can be derived from a data source */ | ||
| public abstract class DataSourceMetaInformation { |
There was a problem hiding this comment.
I'm not fully convinced with the name... Yes, it does describe a data source, but it does not describe a data source in general. It's more describing a time series data source. But TimeSeriesDataSourceMetaInformation is a bit long. Do you have any other suggestion?
There was a problem hiding this comment.
In that case, how about simply TimeSeriesMetaInformation?
src/main/java/edu/ie3/datamodel/io/naming/DataSourceMetaInformation.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/ie3/datamodel/io/naming/timeseries/IndividualTimeSeriesMetaInformation.java
Outdated
Show resolved
Hide resolved
|
|
||
| /** Specific meta information, that can be derived from a individual time series file */ | ||
| public class IndividualTimeSeriesMetaInformation extends FileNameMetaInformation { | ||
| public class IndividualTimeSeriesMetaInformation extends DataSourceMetaInformation { |
There was a problem hiding this comment.
I know, it's not your fault (I introduced this flaw), but I'm not super happy with the fact, that this is not an abstract class. It would be better, if you could only instantiate SqlIndividualTimeSeriesMetaInformation and CsvIndividualTimeSeriesInformation.
One opportunity, I would like you to check out, is to check, if EntityPersistenceNamingStrategy#extractIndividualTimesSeriesMetaInformation couldn't be moved to both of the implementing classes as static method parse(String, ...). Parts of the logic of the method could obviously be moved here and then the individual time series pattern doesn't have to be publicly accessible.
What do you think?
There was a problem hiding this comment.
To the second part: A counter-argument would be here, that multiple EntityPersistenceNamingStrategy can exist and thus allow for different naming strategies of files/tables. When integrating the function into the timeseries meta information, only one single naming strategy could exist.
There was a problem hiding this comment.
To the first point: IndividualTimeSeriesMetaInformation is currently instantiated in EntityPersistenceNamingStrategy#extractIndividualTimesSeriesMetaInformation and I think it makes some sense that way. If we want to make the class abstract, we have to think of another solution there.
|
|
||
| /** Specific meta information, that can be derived from a load profile time series file */ | ||
| public class LoadProfileTimeSeriesMetaInformation extends FileNameMetaInformation { | ||
| public class LoadProfileTimeSeriesMetaInformation extends DataSourceMetaInformation { |
There was a problem hiding this comment.
Same applies as for InvdividualTimeSeriesMetaInformation. ^^
| * Enhancing the {@link IndividualTimeSeriesMetaInformation} with the name of the table containing | ||
| * the time series | ||
| */ | ||
| public static class SqlIndividualTimeSeriesMetaInformation |
There was a problem hiding this comment.
The class nearly has the same outline as CsvIndividualTimeSeriesMetaInformation. Maybe we could also merge both classes? Instead of tableName and fullFilePath something like location?
There was a problem hiding this comment.
I like your suggestion, however I also like having a distinction between csv and sql meta information
| sqlMetaInf.tableName == tableName | ||
| } | ||
|
|
||
| def "SqlIndividualTimeSeriesMetaInformation returns a proper String representation"() { |
There was a problem hiding this comment.
Well, I guess we don't need such kind of a test, right?
There was a problem hiding this comment.
Without it, test coverage is too low
Co-authored-by: Chris Kittl <44838605+ckittl@users.noreply.github.com>
|
Analysis Details0 Issues
Coverage and Duplications
Project ID: edu.ie3:PowerSystemDataModel |
|
This is now continued in #524. Please do not delete the branch here yet. |
sebastian-peter
deleted the
sp/#513-SqlIndividualTimeSeriesMetaInformation
branch
Resolves #513
Additions:
SqlConnector.SqlIndividualTimeSeriesMetaInformationanalogous toCsvFileConnector.CsvIndividualTimeSeriesMetaInformationAdapted:
SqlTimeSeriesMappingSourceRefactorings:
edu.ie3.datamodel.io.csvtoedu.ie3.datamodel.io.naming:FileNameMetaInformation(renamed toDataSourceMetaInformation)edu.ie3.datamodel.io.csv.timeseriestoedu.ie3.datamodel.io.naming.timeseries:IndividualTimeSeriesMetaInformation,LoadProfileTimeSeriesMetaInformation,ColumnScheme