1409 importer handling of imvalid files #1437

abdelr · 2022-01-11T11:53:59Z

msevestre

One suggestion to avoid side effects

msevestre · 2022-01-11T17:58:30Z

src/OSPSuite.Infrastructure.Import/Services/Importer.cs

@@ -113,10 +119,6 @@ public void CalculateFormat(IDataSourceFile dataSource, IReadOnlyList<ColumnInfo
            throw new UnsupportedFormatException(dataSource.Path);

         dataSource.AvailableFormats = AvailableFormats(dataSource.DataSheets[sheetName].RawData, columnInfos, metaDataCategories).ToList();


Looking at this code, we are modifying the dataSource given as parameter. Can I suggest to change this methods so that it does not have side effect and returns the availableFormats instead? Line 108 for instance . It is not clear at all that the format will be set in this method. Thougts?

@abdelr not sure if you saw this

@abdelr not sure if you saw this

Sure, but this is still a work in progress since the last discussion on markers for the sheets

msevestre

This is a big PR with some interesting changes. I need some more time to review

msevestre · 2022-02-02T10:39:35Z

src/OSPSuite.Assets/UIConstants.cs

@@ -1275,6 +1277,9 @@ public static class Error
      public static readonly string NaNOnData = "Data contains NaN values at imported columns. Select a different action for NaN values or clean your data.";
      public static readonly string UnsupportedFileType = "The type of file that you are trying to open is not currently supported";
      public static readonly string CannotRemoveBaseGridColumnStillInUse = "Cannot remove base grid column still used by other columns";
+      public static readonly string SimpleParseErrorMessage = "There were errors while parsing your data. Navigate to the sheets to read the concrete error.";
+
+      public static string ParseErrorMessage(IEnumerable<string> errors) => "There were errors while parsing your data: " + string.Join(". ", errors);


here you could use string interpolation and not the old school +

msevestre

The change in ospsuite.Dimensions is suspicious

msevestre

I see wha't's happening and this is ok. We are breaking a lot of encapsulation but passing Cache back and forth .
Cache is great because you can define a way to retrieve keys automatically by specifing a lambda. We are never using it. WE also never know what the key is. Is it the sheetName sometimes? what's a DataSet compared to a Sheet. Why are we adding DataSet with the sheet key as key..
Lots of questions for me...also coming from older code. We need to run resharper on those files because some basic optimization and code clean up are not done

msevestre · 2022-02-03T15:21:55Z

src/OSPSuite.Infrastructure.Import/Core/DataSource.cs

@@ -16,15 +19,15 @@ public interface IDataSource
   {
      void SetDataFormat(IDataFormat dataFormat);
      void SetNamingConvention(string namingConvention);
-      void AddSheets(Cache<string, DataSheet> dataSheets, IReadOnlyList<ColumnInfo> columnInfos, string filter);
+      Cache<IDataSet, List<ParseErrorDescription>> AddSheets(Cache<string, DataSheet> dataSheets, IReadOnlyList<ColumnInfo> columnInfos, string filter);


This is an unexpected signature. What does this returned cache represent? .Can you comment the interface so that we know what's going on?
In general, returning a cache is not commun weird because you are exposing the API to the consumer but it might be required

msevestre · 2022-02-03T15:22:10Z

src/OSPSuite.Infrastructure.Import/Core/DataSource.cs

      void SetMappings(string fileName, IEnumerable<MetaDataMappingConverter> mappings);
      ImporterConfiguration GetImporterConfiguration();
      IEnumerable<MetaDataMappingConverter> GetMappings();
      Cache<string, IDataSet> DataSets { get; }
      IEnumerable<string> NamesFromConvention();
      NanSettings NanSettings { get; set; }
      ImportedDataSet DataSetAt(int index);
-      void ValidateDataSourceUnits(IReadOnlyList<ColumnInfo> columnInfos);
+      Cache<IDataSet, List<ParseErrorDescription>> ValidateDataSourceUnits(IReadOnlyList<ColumnInfo> columnInfos);


same with this one.

msevestre · 2022-02-03T15:31:55Z

src/OSPSuite.Infrastructure.Import/Core/Exceptions/ParseErrorDescription.cs

+      }
+   }
+
+   public class MismatchingArrayLengthsParseErrorDescription : ParseErrorDescription


Why do we have all those classes instead of using the string? associated with the message?

msevestre · 2022-02-03T15:32:18Z

src/OSPSuite.Infrastructure.Import/Core/Exceptions/ParseErrorDescription.cs

+   {
+      public EmptyDataSetsParseErrorDescription(IEnumerable<string> dataSetNames)
+      {
+         Message = Error.EmptyDataSet($"{string.Join(", ", dataSetNames)}");


use dataSetNames.ToString(", ")

msevestre · 2022-02-03T15:34:56Z

src/OSPSuite.Infrastructure.Import/Core/Helpers/CachedListHelpers.cs

+      }
+
+      public static void Add<T1, T2>(Cache<T1, List<T2>> cache, T1 key, IEnumerable<T2> list)
+      {


Make in an extension method (Call this CachedListExtensions) and the first one should call the other instead of duplicating.

msevestre · 2022-02-03T15:36:19Z

src/OSPSuite.Infrastructure.Import/Core/IDataSet.cs

      {
-         if (_data.Any(dataSet => dataSet.Data.Any(pair => pair.Key.ColumnInfo.IsMandatory && pair.Value.Any(point => point.Measurement == indicator || double.IsNaN(point.Measurement)))))
-            throw new NanException();
+         return _data.Any(dataSet => dataSet.Data.Any(pair => pair.Key.ColumnInfo.IsMandatory && pair.Value.Any(point => point.Measurement == indicator || double.IsNaN(point.Measurement))));


can we comment a bit on this line ...please. And maybe we can break the logic a bit here because it's very complicated to decipher
Also run resharper on this file

msevestre · 2022-02-03T15:36:56Z

src/OSPSuite.Infrastructure.Import/Services/Importer.cs

+         foreach(var x in dataSource.DataSheets.Keys)
+         {
+            dataSource.AvailableFormats = CalculateFormat(dataSource, columnInfos, metaDataCategories, x).ToList();
+            if (dataSource.AvailableFormats?.Count > 0)


How can it be nulll if you call ToList before?

msevestre · 2022-02-03T15:39:01Z

src/OSPSuite.Infrastructure.Import/Services/Importer.cs

-            throw new UnsupportedFormatException(dataSource.Path);
-
-         dataSource.Format = dataSource.AvailableFormats.FirstOrDefault();
+         return AvailableFormats(dataSource.DataSheets[sheetName].RawData, columnInfos, metaDataCategories);


This could return a IReadOnly so that you don't have to TOList earlier.

msevestre · 2022-02-03T15:41:37Z

src/OSPSuite.Infrastructure.Import/Core/DataSource.cs

@@ -16,15 +19,15 @@ public interface IDataSource
   {
      void SetDataFormat(IDataFormat dataFormat);
      void SetNamingConvention(string namingConvention);
-      void AddSheets(Cache<string, DataSheet> dataSheets, IReadOnlyList<ColumnInfo> columnInfos, string filter);
+      Cache<IDataSet, List<ParseErrorDescription>> AddSheets(Cache<string, DataSheet> dataSheets, IReadOnlyList<ColumnInfo> columnInfos, string filter);
      void SetMappings(string fileName, IEnumerable<MetaDataMappingConverter> mappings);
      ImporterConfiguration GetImporterConfiguration();
      IEnumerable<MetaDataMappingConverter> GetMappings();
      Cache<string, IDataSet> DataSets { get; }


I don't like this as a cache exposed. DataSet are added outside of the DataSource as it stands

msevestre · 2022-02-03T15:42:46Z

src/OSPSuite.Infrastructure.Import/Core/DataSource.cs

         _importer.AddFromFile(_configuration.Format, filterSheets(dataSheets, filter), columnInfos, this);
         if (NanSettings == null || !double.TryParse(NanSettings.Indicator, out var indicator))
            indicator = double.NaN;
+         var errors = new Cache<IDataSet, List<ParseErrorDescription>>();
         foreach (var dataSet in DataSets.KeyValues)


why are you iterating over the keyValues? You are never using the Key

abdelr · 2022-02-04T14:24:31Z

@msevestre I encapsulated the errors on a new class. I hope it makes more sense now?

msevestre · 2022-02-04T15:30:17Z

src/OSPSuite.Infrastructure.Import/Core/DataSource.cs

      }

-      void IDataSource.ValidateDataSourceUnits(IReadOnlyList<ColumnInfo> columnInfos)
+      ParseErrors IDataSource.ValidateDataSourceUnits(IReadOnlyList<ColumnInfo> columnInfos)


Why the IDataSource. here?

msevestre · 2022-02-04T15:30:46Z

src/OSPSuite.Infrastructure.Import/Core/Helpers/CachedListHelpers.cs

+
+namespace OSPSuite.Infrastructure.Import.Core.Helpers
+{
+   static public class CachedListHelpers


This needs to be removed right?

msevestre · 2022-02-04T15:31:16Z

src/OSPSuite.Infrastructure.Import/Core/IDataSet.cs

-         if (_data.Any(dataSet => dataSet.Data.Any(pair => pair.Key.ColumnInfo.IsMandatory && pair.Value.Any(point => point.Measurement == indicator || double.IsNaN(point.Measurement)))))
-            throw new NanException();
+         //returns true if for any ParsedDataSet 
+         return _data.Any(


msevestre · 2022-02-04T15:31:53Z

src/OSPSuite.UI/Properties/licenses.licx

@@ -0,0 +1,3 @@
+DevExpress.XtraEditors.CheckEdit, DevExpress.XtraEditors.v21.2, Version=21.2.3.0, Culture=neutral, PublicKeyToken=b88d1754d700e49a


remove this file

msevestre · 2022-02-04T15:32:28Z

src/OSPSuite.UI/Views/Importer/ImporterDataView.cs

@@ -222,13 +228,12 @@ public string GetActiveFilterCriteria()

      public void AddTabs(List<string> sheetNames)
      {
-         //we should seek an alternative
+         importerTabControl.Images = imageCollection1;


rename this control

msevestre · 2022-02-04T15:32:49Z

src/OSPSuite.UI/Views/Importer/ImporterDataView.cs

+         if (_lastLoadedDataSets.Contains(SelectedTab) && _lastErrors.Contains(_lastLoadedDataSets[SelectedTab]))
+         {
+            labelControlError.Text = Error.ParseErrorMessage(_lastErrors.ErrorsFor(_lastLoadedDataSets[SelectedTab]).Select(x => x.Message));
+            layoutControlItem2.Visibility = DevExpress.XtraLayout.Utils.LayoutVisibility.Always;


Rename layoutControlItem2
Also use resharper to simplify some of the name space stuff

msevestre · 2022-02-04T15:34:03Z

src/OSPSuite.UI/Views/Importer/ImporterDataView.resx

+    <value>163, 17</value>
+  </metadata>
+  <assembly alias="DevExpress.Utils.v21.2" name="DevExpress.Utils.v21.2, Version=21.2.3.0, Culture=neutral, PublicKeyToken=b88d1754d700e49a" />
+  <data name="imageCollection1.ImageStream" type="DevExpress.Utils.ImageCollectionStreamer, DevExpress.Utils.v21.2" mimetype="application/x-microsoft.net.object.bytearray.base64">


We do not use images like that at all. Icons are added to the ImageList and are loaded all dynamically

msevestre

The domain codes is SO Much better. Thanks
The UI code needs to be updated :
Some controls have default naming with 1 at the end
Not proper usage of icons

msevestre · 2022-02-04T15:37:08Z

src/OSPSuite.UI/Views/Importer/ImporterDataView.cs

+      private void refreshErrorMessage()
+      {
+
+         if (_lastLoadedDataSets.Contains(SelectedTab) && _lastErrors.Contains(_lastLoadedDataSets[SelectedTab]))


somehow this feels like this code belongs in the presenter.

msevestre · 2022-02-04T15:37:53Z

src/OSPSuite.UI/Views/Importer/ImporterDataView.cs

+      {
+         importerTabControl.TabPages.Each(x =>
+         {
+            if (_lastLoadedDataSets.Contains(x.Text))


this codes for which image to show also belongs in the presenter
And hopefully, we won't have those magin number anymore 1 and 0 once we use ApplicationIcons

msevestre · 2022-02-04T15:38:26Z

src/OSPSuite.UI/Views/Importer/ImporterDataView.resx

    <value>17, 17</value>
+  </metadata>
+  <data name="useForImportCheckEdit.ToolTip" xml:space="preserve">
+    <value>When selected, the filter will apply to the data during the import process. When deselected, the filter only affects this view. Check documentation for more information on defining filters: &lt;href=https://docs.open-systems-pharmacology.org/shared-tools-and-example-workflows/features-of-tables#filtering&gt;https://docs.open-systems-pharmacology.org/shared-tools-and-example-workflows/features-of-tables#filtering&lt;/href&gt;</value>


Tootip needs to be put into constants!

msevestre · 2022-02-07T17:17:23Z

src/OSPSuite.Infrastructure.Import/Core/DataSource.cs

+         var accumulatedIndexes = 0;
+         while (sheet.MoveNext() && index >= 0)
+         {
+            if (sheet.Current.Data.Count() > index)


(sheet.Current.Data.Count() 3 times in the same while loop. Maybe cache it?

abdelr · 2022-02-09T07:43:38Z

@msevestre Only open PR. Just adding you to the comment to make sure you know this is ready for revision.

msevestre · 2022-02-09T08:14:04Z

src/OSPSuite.Infrastructure.Import/Core/DataSource.cs

@@ -117,7 +155,27 @@ public IEnumerable<string> NamesFromConvention()
         return _importer.NamesFromConvention(_configuration.NamingConventions, _configuration.FileName, DataSets, _mappings);
      }

-      public ImportedDataSet DataSetAt(int index)
+      public IDataSet DataSetAt(int index)


Let's talk about this in the meeting. I don't understand what this is doing.

msevestre · 2022-02-09T11:06:04Z

I think this is working as expected. I have my concerns regarding the deeply nested structure in DataSource.cs
I am going to merge this however and this can be addressed at some other time. I will create an issue for this

abdelr added 2 commits January 11, 2022 11:39

Should consider columns with string values

b728d54

based format on first valid sheet

3f308dc

abdelr added the Importer Observed data importer label Jan 11, 2022

abdelr requested review from msevestre, Yuri05 and georgeDaskalakis January 11, 2022 11:53

abdelr self-assigned this Jan 11, 2022

Fixing tests

e53b503

msevestre reviewed Jan 11, 2022

View reviewed changes

Collecting errors instead of throwing on first

0c922de

abdelr marked this pull request as draft January 15, 2022 20:51

abdelr added 4 commits January 17, 2022 11:48

Showing error messages and marks

a219c50

Updating error message

50be51b

Fixing test

1421907

CalculateFormat does not have side effects

86eacc7

abdelr requested a review from msevestre January 28, 2022 13:19

abdelr marked this pull request as ready for review January 28, 2022 13:20

abdelr marked this pull request as draft January 28, 2022 14:01

Fixing tests

d76ef41

abdelr marked this pull request as ready for review January 28, 2022 14:14

msevestre reviewed Feb 2, 2022

View reviewed changes

msevestre requested changes Feb 3, 2022

View reviewed changes

abdelr added 7 commits February 4, 2022 09:59

Using string interpolation

2a41306

Changing return type from Cache to a new class

aa1c1f8

Misc changes

4ea57b1

Fixing test

cb2c8a3

Merge branch 'develop' into 1409_importer_handling_of_imvalid_files

3a58ecb

fixing tests

066441e

restoring test

bb32d68

abdelr requested a review from msevestre February 4, 2022 14:22

msevestre reviewed Feb 4, 2022

View reviewed changes

msevestre requested changes Feb 4, 2022

View reviewed changes

msevestre reviewed Feb 4, 2022

View reviewed changes

abdelr added 2 commits February 7, 2022 18:08

Using dynamic images plus misc fixes

72834f9

Cleaning resource file

f6b6adc

msevestre reviewed Feb 7, 2022

View reviewed changes

abdelr added 3 commits February 7, 2022 20:12

missing changes

70ba716

Moving logic to presenter

dd693a3

Proper initialization and resources

566ca79

abdelr requested a review from msevestre February 7, 2022 21:06

msevestre reviewed Feb 9, 2022

View reviewed changes

msevestre merged commit 428ef3a into develop Feb 9, 2022

msevestre deleted the 1409_importer_handling_of_imvalid_files branch February 9, 2022 11:06

		@@ -113,10 +119,6 @@ public void CalculateFormat(IDataSourceFile dataSource, IReadOnlyList<ColumnInfo
		throw new UnsupportedFormatException(dataSource.Path);

		dataSource.AvailableFormats = AvailableFormats(dataSource.DataSheets[sheetName].RawData, columnInfos, metaDataCategories).ToList();

		@@ -0,0 +1,3 @@
		DevExpress.XtraEditors.CheckEdit, DevExpress.XtraEditors.v21.2, Version=21.2.3.0, Culture=neutral, PublicKeyToken=b88d1754d700e49a

1409 importer handling of imvalid files #1437

1409 importer handling of imvalid files #1437

Conversation

abdelr commented Jan 11, 2022 • edited Loading

msevestre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msevestre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msevestre left a comment

Choose a reason for hiding this comment

msevestre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdelr commented Feb 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msevestre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdelr commented Feb 9, 2022

Choose a reason for hiding this comment

msevestre commented Feb 9, 2022

abdelr commented Jan 11, 2022 •

edited

Loading