Add files as argument in CLI

[domna]

Fixes #238

• Supports adding files via dataconverter --reader my_reader --nxdl NXmynxdl filename1 filename2.
• You can also pass a folder and it will recursively get all the files in it and pass it to the reader function.
• Shell pattern replacement works out of the box dataconverter --reader my_reader --nxdl NXmynxdl *.h5 or dataconverter ... my_folder/**/* for files in folders
• Keeps the old --input-file option but shows a deprecation warning.

[domna]

I set the whole Area-B team in review so you are aware of this change. Please give it a run and see if it works well for you

[coveralls]

Pull Request Test Coverage Report for Build 7991873551

Details

• 29 of 37 (78.38%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.2%) to 43.708%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pynxtools/dataconverter/convert.py	16	24	66.67%

Totals
Change from base Build 7989122601:	0.2%
Covered Lines:	4738
Relevant Lines:	10840

---

💛 - Coveralls

[RubelMozumder]

LGTM, I have two points to mention here.

1. Could we also keep the old syntax --input-file, alongside with the new way of converter, but given a warning that we are going replace the syntax with the new sytax. If you implement it directly all the NORTH will get infected and make the users confused.
2. Do you think to keep --input-file only for once for all the given input files. Which would be intuitive, what is the input file and what is the nxdl.

[RubelMozumder]

I do not see any reflection on the test. The test can also be modified if it is out of mind but needed.

[domna]

I removed the --input-file inputs except for test_cli as we cannot test this like this. click is already checking that the file we pass is existing, therefore we never start parsing there. But I think it's fine to still test the option there.

[RubelMozumder]

There are some --input-file options in the test file and they can be removed.

[domna]

No the only occurrence left is

pynxtools/tests/dataconverter/test_convert.py

Lines 126 to 127 in f982c0e

	elif "--input-file" in cli_inputs:
	assert "test_input" in caplog.text

which cannot be removed, because click automatically checks if test_input exists (and it doesn't). We can remove this test entirely but I would keep it until we completely remove --input-file.

[domna]

LGTM, I have two points to mention here.

1. Could we also keep the old syntax --input-file, alongside with the new way of converter, but given a warning that we are going replace the syntax with the new sytax. If you implement it directly all the NORTH will get infected and make the users confused.

This is how it's implemented. I just removed all documentation of the --input-file as we would want users to use the simpler syntax. I will implement a warning that --input-file will be deprecated eventually. Good idea!

2. Do you think to keep --input-file only for once for all the given input files. Which would be intuitive, what is the input file and what is the nxdl.

I don't understand what you mean. The nxdl is clearly specified by --nxdl NXmynxdl so I think it should be obvious which argument is the nxdl.

[lukaspie]

2. Do you think to keep --input-file only for once for all the given input files. Which would be intuitive, what is the input file and what is the nxdl.

I don't understand what you mean. The nxdl is clearly specified by --nxdl NXmynxdl so I think it should be obvious which argument is the nxdl.

If I understand @RubelMozumder correctly, you want to still keep --input-file and then allow to pass multiple file? That would be very confusing. It would need to be changed to --input-files at least and I don't think there is big improvement over just having no flag (like what Florian is proposing here).

[domna]

2. Do you think to keep --input-file only for once for all the given input files. Which would be intuitive, what is the input file and what is the nxdl.

I don't understand what you mean. The nxdl is clearly specified by --nxdl NXmynxdl so I think it should be obvious which argument is the nxdl.

If I understand @RubelMozumder correctly, you want to still keep --input-file and then allow to pass multiple file? That would be very confusing. It would need to be changed to --input-files at least and I don't think there is big improvement over just having no flag (like what Florian is proposing here).

Yes, I agree with you @lukaspie. In this case the direct file arguments should be used (it should also work now with shell pattern replacement, e.g., something like dataconverter *.h5 should only run for all the h5 files in the folder). This is also much more aligned with how common cli tools work.

[lukaspie]

3. Do you think to keep --input-file only for once for all the given input files. Which would be intuitive, what is the input file and what is the nxdl.

I don't understand what you mean. The nxdl is clearly specified by --nxdl NXmynxdl so I think it should be obvious which argument is the nxdl.

If I understand @RubelMozumder correctly, you want to still keep --input-file and then allow to pass multiple file? That would be very confusing. It would need to be changed to --input-files at least and I don't think there is big improvement over just having no flag (like what Florian is proposing here).

Yes, I agree with you @lukaspie. In this case the direct file arguments should be used (it should also work now with shell pattern replacement, e.g., something like dataconverter *.h5 should only run for all the h5 files in the folder). This is also much more aligned with how common cli tools work.

Great, then we all agree to use positional arguments. I will test now with the XPS reader and the data that I have here locally.

[sherjeelshabih]

I just checked it for a couple of runs of the dataconverter I had in my terminal history. They work well.

I just have one suggestion. In the --input-file help text, can you bring the Deprecated warning before the actual doc? This way it's more obvious for someone who is quickly doing a --help and not reading thoroughly. It is at the end currently without any space after the bracket.

--input-file TEXT               The path to the input data file to read.
                                  (Repeat for more than one file.)Deprecated:
                                  Please use the positional file arguments
                                  instead.

[domna]

I just checked it for a couple of runs of the dataconverter I had in my terminal history. They work well.

I just have one suggestion. In the --input-file help text, can you bring the Deprecated warning before the actual doc? This way it's more obvious for someone who is quickly doing a --help and not reading thoroughly. It is at the end currently without any space after the bracket.

--input-file TEXT               The path to the input data file to read.
                                  (Repeat for more than one file.)Deprecated:
                                  Please use the positional file arguments
                                  instead.

I change it, now it looks like this:

 --input-file TEXT               Deprecated: Please use the positional file
                                  arguments instead. The path to the input
                                  data file to read. (Repeat for more than one
                                  file.)

[RubelMozumder]

I approved it, if all are fixed it is ready to merge.

[lukaspie]

As discussed with Florian already, making --nxdl required breaks the usage of params-file. When I use dataconverter --params-file params.yaml, it complains that you need to pass an nxdl.

[sherjeelshabih]

As discussed with Florian already, making --nxdl required breaks the usage of params-file. When I use dataconverter --params-file params.yaml, it complains that you need to pass an nxdl.

Yeah. That's why I added that custom check. I see that Florian has already replaced the IOError with clicks own set. This should be fine with having --nxdl not required in click's config.

[lukaspie]

LGTM, works for the XPS reader too