Zadanie #5275

Code review: moduł corpusio

Added by Adam Radziszewski over 9 years ago. Updated over 9 years ago.

Status:ZamkniętyStart date:27 Mar 2014
Priority:NormalnyDue date:
Assignee:Adam Radziszewski% Done:

100%

Category:-
Target version:python

Description

Interfejs modułu corpusio:
  • Interfejs get_reader i get_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 albo get_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

Also available in: Atom PDF