Skip to content

Conversation

@MichaelStritt
Copy link
Contributor

Linked issue

Check out #940

@MichaelStritt MichaelStritt linked an issue Nov 29, 2021 that may be closed by this pull request
@MichaelStritt MichaelStritt added the import Related to data import module label Nov 29, 2021
@MichaelStritt MichaelStritt self-assigned this Nov 29, 2021
@MichaelStritt MichaelStritt requested review from jan-petr and removed request for HenkMutsaerts November 29, 2021 20:53
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides XML, I have one issue:

  1. What if the Extension is given without the fullstop - just "PAR", but then just "PAR" can be mistaken with something else. But we can't look at it as the last string "PAR$" cause there can be some bracket etc.

I have no real idea how to solve this. Better to send to Henk for review as well - even though it will take some time...

Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with it and let's see if Henk has any idea how to address my comment in PR...

Copy link
Member

@HenkMutsaerts HenkMutsaerts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add an elseif for another (unknown) extension, then it can just issue a warning that we detected an unknown extension.

MINOR:
I would replace the regular expressions by a single one, then you can do the regexp once outside the if statement.

hasKnownExtension = ~isempty(regexpi(lastElement,'.(dcm|ima|xml|par|rec)'));

and you can combine else and if on line 84 into elseif.

Otherwise nice!

@HenkMutsaerts
Copy link
Member

Besides XML, I have one issue:

  1. What if the Extension is given without the fullstop - just "PAR", but then just "PAR" can be mistaken with something else. But we can't look at it as the last string "PAR$" cause there can be some bracket etc.

I have no real idea how to solve this. Better to send to Henk for review as well - even though it will take some time...

Try using Matlab's fileparts for this. This should separate the content of the last folder level for extension. Check if it gives an empty file or empty extension properly.

@jan-petr jan-petr self-requested a review December 1, 2021 20:27
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small edit needed

Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@MichaelStritt MichaelStritt merged commit a241e0a into develop Dec 2, 2021
@MichaelStritt MichaelStritt deleted the import-#940_bMatch branch December 2, 2021 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

import Related to data import module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Managing bMatchDirectories is false

4 participants