search

kbss-cvut / s-pipes

Tool for execution of RDF-based pipelines.
GNU Lesser General Public License v3.0
4 stars 5 forks source link

Add merged cells metadata new #226

Closed blcham closed 10 months ago

blcham commented 10 months ago

@rodionnv Please: 1) add correct html expected-output.ttl and make sure that excel and HTML tables are basically same 2) implement getTableName, getNumberTables methods for HTML

rodionnv commented 10 months ago

@blcham For some reason I can't request your review on this Pull Request but I believe that problem with HTML tables is solved.

blcham commented 10 months ago

@rodionnv

Looks fine, but i do not see implemented method getTableName() for HML2CSVConverter. If you implement it you can make excel-expected-output.ttl and html-expected-output.ttl same, i.e. include only expected-output.ttl file.

BTW, you cannot assign me for review, because it is not possible to assign person that created pull request :(

rodionnv commented 10 months ago

@blcham I have implemented missing methods in HML2TSVConvertor. But method getNumberTables() is not used now because processing of multiple HTML tables is not supported (because HML2TSVConvertor just process all html rows despite the fact that they may be from different tables). For that same reason method getTableName() returns the name of the first found table. I suppose that it will require some major changes to make it possible to process different tables from the same html file (like it's done in XLS2TSVConvertor)

blcham commented 10 months ago

@blcham I have implemented missing methods in HML2TSVConvertor. But method getNumberTables() is not used now because processing of multiple HTML tables is not supported (because HML2TSVConvertor just process all html rows despite the fact that they may be from different tables). For that same reason method getTableName() returns the name of the first found table. I suppose that it will require some major changes to make it possible to process different tables from the same html file (like it's done in XLS2TSVConvertor)

I do not see why we are treating XLS differently from HTML -- the first table in HTML is "same-as" first sheet in XLS. The simplest solution would be to require that property process-table-at-index would have to be set to 1 and otherwise throw notImplementedException. What do you think ?

rodionnv commented 10 months ago

@blcham I have implemented missing methods in HML2TSVConvertor. But method getNumberTables() is not used now because processing of multiple HTML tables is not supported (because HML2TSVConvertor just process all html rows despite the fact that they may be from different tables). For that same reason method getTableName() returns the name of the first found table. I suppose that it will require some major changes to make it possible to process different tables from the same html file (like it's done in XLS2TSVConvertor)

I do not see why we are treating XLS differently from HTML -- the first table in HTML is "same-as" first sheet in XLS. The simplest solution would be to require that property process-table-at-index would have to be set to 1 and otherwise throw notImplementedException. What do you think ?

Okay, I agree, I will make this changes.

rodionnv commented 10 months ago

@blcham I have implemented missing methods in HML2TSVConvertor. But method getNumberTables() is not used now because processing of multiple HTML tables is not supported (because HML2TSVConvertor just process all html rows despite the fact that they may be from different tables). For that same reason method getTableName() returns the name of the first found table. I suppose that it will require some major changes to make it possible to process different tables from the same html file (like it's done in XLS2TSVConvertor)

I do not see why we are treating XLS differently from HTML -- the first table in HTML is "same-as" first sheet in XLS. The simplest solution would be to require that property process-table-at-index would have to be set to 1 and otherwise throw notImplementedException. What do you think ?

Okay, I agree, I will make this changes.

@blcham Done.

blcham commented 10 months ago

@rodionnv congrats! I merged this one.