Zadanie #5275
Code review: moduł corpusio
Status: | Zamknięty | Start date: | 27 Mar 2014 | |
---|---|---|---|---|
Priority: | Normalny | Due date: | ||
Assignee: | Adam Radziszewski | % Done: | 100% | |
Category: | - | |||
Target version: | python |
Description
- Interfejs
get_reader
iget_writer
wyglądają dobrze - Funkcja
get_named_tagset
jest myląca: tak nazywa się funkcja z Corpus2, która zwraca obiekt tagsetu na podstawie nazwy tagsetu (np. "nkjp"), gdzie obiekty klasy Tagset są ceche'owane. Proponuję to nazwaćget_tagset
alboget_tagset_from_config
czy jakoś podobnie. - Interfejs tej funkcji: znowu mamy tę
std::map
. Być może konfigi będą reprezentowane docelowo przez taką mapę, ale wtedy warto będzie chyba ją opakować w klasę albo przynajmniej zrobić typedefa i nazwać go tak, by było wiadomo, że chodzi o typ przeznaczony na konfigi. - Przy text2mask tego stringa text chyba można przekazać przez referencję.
Implementacja modułu corpusio wygląda ładnie.
History
#1 Updated by Adam Radziszewski over 9 years ago
- % Done changed from 0 to 80
Zasadnicza uwaga: dlaczego nie używasz redmine'a? Gdy poprawisz coś związanego z którymś zagadnieniem, to zaktualizuj proszę zagadnienie. W szczególności gdy wykonasz zadanie, to ustaw status na Wykonany. Jeśli masz po drodze uwagi związane z konkretnym zadaniem, to komentuj proszę w ramach zadania, to naprawdę jest ważne, bez tego trudno mi zapanować nad wszystkimi działaniami.
Widzę, że funkcja pobierająca tagset ma już nową nazwę i typ na konfig się pojawił.
Mógłbyś przypomnieć na czym polegał problem z const std::string & w text2mask
(tj. dlaczego nie dałeś w końcu referencji const)?
Jeśli dobrze kojarzę, to obecnie problem polega na tym, że zamiana odbywa się na miejscu: boost::algorithm::replace_all(text, ",", "-")
.
To rozwiązanie w sumie nic nie daje, bo i tak na wejściu do funkcji tworzona jest lokalna kopia tego stringa, na której dokonwywana jest zamiana.
Jeśli dobrze myślę, to można dać const ref jako argument funkcji i w środku stworzyć kopię za pomocą wywołania boost::algorithm::replace_all_copy
.
#2 Updated by Radosław Warzocha over 9 years ago
- Assignee changed from Radosław Warzocha to Adam Radziszewski
- % Done changed from 80 to 100
Wybacz niekorzystanie z redmine. Próbowałem skończyć wszystko jak najbardziej na czas i zwyczajnie zapomniałem. Odtąd będę o tym pamiętał.
Problem z text2mask
był dokładnie taki jak opisałeś (modyfikacja w miejscu). Nie wiedziałem o istnieniu replace_all_copy
. Poprawka została już wprowadzona i wrzucona do repo.
#3 Updated by Radosław Warzocha over 9 years ago
- Status changed from Nowy to Gotowy
#4 Updated by Adam Radziszewski over 9 years ago
- Status changed from Gotowy to Zamknięty