persephone-tools / persephone

A tool for automatic phoneme transcription
Apache License 2.0
155 stars 26 forks source link

[MRG] Make optional arguments keyword args #216

Closed shuttle1987 closed 5 years ago

shuttle1987 commented 5 years ago

Refactor argument handling for corpus initialization, Closes #186

shuttle1987 commented 5 years ago

Note that this is a breaking change.

The change is to this:

    def __init__(self, feat_type: str, label_type: str, tgt_dir: Path,
                 labels: Optional[Set[str]] = None,
                 *,
                 max_samples: int=1000,
                 speakers: Optional[Sequence[str]] = None) -> None:

But given we are already making a breaking change I'd rather it were this:

    def __init__(self, feat_type: str, label_type: str, tgt_dir: Path,
                 *,
                 labels: Optional[Set[str]] = None,
                 max_samples: int=1000,
                 speakers: Optional[Sequence[str]] = None) -> None:
shuttle1987 commented 5 years ago

This is the sort of thing we should merge sooner rather than later since it will give us much more flexibility with how the API is used going forward.

oadams commented 5 years ago

Okay, do you think I should merge it in now? Not sure because of the WIP tag in the title.

shuttle1987 commented 5 years ago

I made those changes, test are not updated just yet. Note that this has the capacity to break things so a version bump is probably in order if this gets merged.

shuttle1987 commented 5 years ago

I think this is good to go now.