Title: Check invalid config name
Author: lhoestq (MEMBER)
State: closed
Created: 2020-05-26T13:52:51Z

As said in #194, we should raise an error if the config name has bad characters.
Bad characters are those that are not allowed for directory names on windows.

Comments:
I think that's not related to the config name but the filenames in the dummy data. Mostly it occurs with files downloaded from drive. In that case the dummy file name is extracted from the google drive link and it corresponds to what comes after `https://drive.google.com/`



> I think that's not related to the config name but the filenames in the dummy data. Mostly it occurs with files downloaded from drive. In that case the dummy file name is extracted from the google drive link and it corresponds to what comes after `https://drive.google.com/`

The filenames of the dummy data are now encoded (see #173). So this is not a problem anymore.

The problem here is different and comes from the directory names where we save the arrow files (basically `dataset_name/config_name/version`). In this case we could have invalid directory names because of the config name


Okay great then.

I like the method, but I'm wondering whether it should just be a test method instead of a `__post_init__` function. From a logical point of view the only reason this error would be thrown is because of an invalid config name introduced when creating the dataset script / adding a new dataset => so I think it might be better to write a simple test for this in `test_dataset_common.py`...what do you think @lhoestq ?

`test_dataset_common.py` only tests canonical datasets no ? What if users wants to create their own script ?

> `test_dataset_common.py` only tests canonical datasets no ? What if users wants to create their own script ?

It tests all dataset that can be loaded either locally or on AWS (which includes all non-canonical datasets as well)...by their own script you mean like a private dataset script that they don't want to be public? I guess even then they could locally run the test functions to check...

We could have a bunch of simple consistency tests that run before uploading with the CLI (without loading data if we don't want to force the user to have dummy data)?

Let's say someone want to create his own private script. As the script is not meant to be shared, it's not going to be placed in `/datasets` right ? Maybe the script is going to be inside another project. If I'm not wrong in this case the `test_dataset_common.py` is not going to test his script.

Raising an error in the post init is a sanity check that would tell the user immediately what's wrong.
The error is raised if he tried to load the script or if he uses `nlp-cli test`

> Let's say someone want to create his own private script. As the script is not meant to be shared, it's not going to be placed in `/datasets` right ? Maybe the script is going to be inside another project. If I'm not wrong in this case the `test_dataset_common.py` is not going to test his script.
> 
> Raising an error in the post init is a sanity check that would tell the user immediately what's wrong.
> The error is raised if he tried to load the script or if he uses `nlp-cli test`

OK, fair point! I'm good with this then :-) 

I'm fine with this as well (even though I understand what you meant @patrickvonplaten, we can still change it later if needed)

> We could have a bunch of simple consistency tests that run before uploading with the CLI (without loading data if we don't want to force the user to have dummy data)?

Yes! I guess that's a big question whether we should force the user to add dummy data. It's probably too tedious for the user...so when uploading to circle ci should we just check 
- 1) All configs can be instantiated (if there are any)
- 2) The BuilderClass can be instantiated ... 
- 3) ... maybe some more

and maybe suggest to the user to add dummy data using the dummy data command?

I really like that we have a test with dummy data for canonical datasets. This is insurance that they'll keep working in the long run. 

On the other hand I understand that we will probably not force this practice for scripts uploaded on S3 by a user under his namespace (non-canonical), as it is tedious. As I understand right now the test is done for all the datasets on aws, even the non-canonical ? We should think about different tests for non-canonical datasets.

I also like the idea of a simple consistency test !

Merging this one for now, we can think about the test for non-canonical datasets later