Skip to content

Conversation

DimitriPapadopoulos
Copy link
Collaborator

@DimitriPapadopoulos DimitriPapadopoulos commented Feb 23, 2023

Rationale:

  1. The -D command line option can be specified multiple times. It makes sense to be able to specify the respective config file option dictionary multiple times too.
  2. While specifying an option multiple times might be an error, setting strict to True catches duplicates only within a single INI file, not across multiple INI files. If we are serious about handling duplicate options, we should therefore handle that manually, distinguishing between options that can be duplicated or not.

Fixes #2727.

Rationale:
1. The `-D` command line option can be specified multiple times.
   It makes sense to be able to specify the respective config file
   option `dictionary` multiple times too.
2. While specifying an option multiple times might be an error,
   setting strict to `True` catches duplicates only within a single
   INI file, not across multiple INI files. If we are serious about
   handling duplicate options, we should therefore handle that
   manually, distinguishing options that can be duplicated or not.
@DimitriPapadopoulos DimitriPapadopoulos force-pushed the dictionary_DuplicateOptionError branch from 2524aa0 to 30fc670 Compare February 23, 2023 19:23
@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as ready for review February 23, 2023 19:24
@larsoner
Copy link
Member

Worth adding a test that fails on main but passes here?

@DimitriPapadopoulos
Copy link
Collaborator Author

DimitriPapadopoulos commented Feb 23, 2023

Indeed, we could add a test with multiple dictionary lines in a config file.

@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as draft February 24, 2023 06:57
@DimitriPapadopoulos
Copy link
Collaborator Author

Besides, it doesn't even work because the underlying data structure is still a dict: in case of duplicate keys, the new value overwrites the old value, which makes sense but is not what we would like here. I will try a few things, such as:

dictionary = (dictionary1.txt, dictionary2.txt)

@yarikoptic
Copy link
Contributor

an alternative could be specification via , but also there is #1980 which offers to list on separate lines - IMHO might be a better choice

@DimitriPapadopoulos
Copy link
Collaborator Author

DimitriPapadopoulos commented Feb 25, 2023

#1980 does not just concatenate lines as claimed, instead it ",".join(lines). This will work with option -S/--skip which expects a “comma-separated list of files to skip”, but not with option -D/--dictionary. Therefore, it is not an alternative, we would have to modify option -D/--dictionary to accept a “comma-separated list of dictionaries” anyway.

@DimitriPapadopoulos
Copy link
Collaborator Author

Replaced by #2767, a better alternative (see #2727 (comment)).

@DimitriPapadopoulos DimitriPapadopoulos deleted the dictionary_DuplicateOptionError branch March 3, 2023 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiple dictionaries (add default one) for dictionary config setting

3 participants