fix(4.traits): реворк задания по парсеру

smirnovei commented

2026-03-22 08:11:46 +00:00

(Migrated from git.csit.sgu.ru)

Не уверен, что это хороший вариант, т.к. выходит много, возможно, слишком много. Но мб будет нормально, если какие-то моменты убрать или добавить бонус за задание или уже все хорошо и надо просто текст написать.
Хочу получить фидбэк до того как начну переписывать текст задания (и typst шаблон, пожалуйста 👉👈)

@nrydanov

Не уверен, что это хороший вариант, т.к. выходит много, возможно, слишком много. Но мб будет нормально, если какие-то моменты убрать или добавить бонус за задание или уже все хорошо и надо просто текст написать. Хочу получить фидбэк до того как начну переписывать текст задания (и typst шаблон, пожалуйста 👉👈) ` @nrydanov`

smirnovei commented

2026-03-27 17:11:56 +00:00

(Migrated from git.csit.sgu.ru)

Да, шаблон нужно дорабатывать и возможно использование #task не самая лучшая идея, но нужен фидбэк и время, которого на следующей неделе не будет из-за большого количества дедлайнов по домашкам.

Да, шаблон нужно дорабатывать и возможно использование \#task не самая лучшая идея, но нужен фидбэк и время, которого на следующей неделе не будет из-за большого количества дедлайнов по домашкам.

rydanovns commented

2026-03-29 06:07:44 +00:00

(Migrated from git.csit.sgu.ru)

Это оверинжиниринг. У тебя фабрика парсеров точно не будет иметь больше одной реализации. Как следствие, сама абстракция не нужна.

rydanovns commented

2026-03-29 06:08:51 +00:00

(Migrated from git.csit.sgu.ru)

Аналогично тут. Я могу представить Parser как трейт, но не понимаю как может быть несколько реализаций фабрики для парсеров файлов.

rydanovns commented

2026-03-29 06:12:00 +00:00

(Migrated from git.csit.sgu.ru)

Почему это называется Errors, а не Error?

rydanovns commented

2026-03-29 06:12:25 +00:00

(Migrated from git.csit.sgu.ru)

по-моему вместо этого можно #[derive(Debug)] сделать.

rydanovns commented

2026-03-29 06:13:55 +00:00

(Migrated from git.csit.sgu.ru)

Почему не pub enum ParserError, где тип будет кодироваться как enum?

rydanovns commented

2026-03-29 06:15:30 +00:00

(Migrated from git.csit.sgu.ru)

Тут TODO лучше написать внутри {} возле CsvParser и TsvParser

rydanovns commented

2026-03-29 06:16:56 +00:00

(Migrated from git.csit.sgu.ru)

Давай задавать тут правильные ограничения. Если надо я потом уберу, а если задания сдадут с нужными ограничениями но не смогут объяснить почему — будет плохо)

rydanovns commented

2026-03-29 06:21:36 +00:00

(Migrated from git.csit.sgu.ru)

Я подумал тут насчет итераторов и сейчас это выглядит плохо, потому что нам примерно никогда понадобится только часть данных из итератора (а это в основном то, из-за чего итераторы любят). Плюс такая сигнатура даст неочевидные ограничения на лайфтайм. Короче, плюсов не вижу, а вот минусы скорее да.

Лучше возвращать Vec<Table>, в случае чего у него есть IntoIterator.

Я подумал тут насчет итераторов и сейчас это выглядит плохо, потому что нам примерно никогда понадобится только часть данных из итератора (а это в основном то, из-за чего итераторы любят). Плюс такая сигнатура даст неочевидные ограничения на лайфтайм. Короче, плюсов не вижу, а вот минусы скорее да. Лучше возвращать `Vec<Table>`, в случае чего у него есть `IntoIterator`.

rydanovns commented

2026-03-29 06:32:22 +00:00

(Migrated from git.csit.sgu.ru)

По хорошему это надо смержить отдельно, мне трудно ревьюить, потому что diff — полное задание, а не только изменения самого задания.

Заметь, что в реальной разработке будут аналогичные проблемы. Нужно стараться, чтобы diff в точности отражал содержимое изменений.

По хорошему это надо смержить отдельно, мне трудно ревьюить, потому что diff — полное задание, а не только изменения самого задания. Заметь, что в реальной разработке будут аналогичные проблемы. Нужно стараться, чтобы diff в точности отражал содержимое изменений.

rydanovns commented

2026-03-29 06:33:48 +00:00

(Migrated from git.csit.sgu.ru)

Review: Changes requested

Правок много, думаю это не исчерпывающий список, но пока мы существующие проблемы не решим, будет трудно вносить правки дальше.

**Review:** Changes requested Правок много, думаю это не исчерпывающий список, но пока мы существующие проблемы не решим, будет трудно вносить правки дальше.

rydanovns commented

2026-03-29 06:34:43 +00:00

(Migrated from git.csit.sgu.ru)

На Typst фидбек готов дать в рамках отдельного MR.

rydanovns commented

2026-03-29 06:39:47 +00:00

(Migrated from git.csit.sgu.ru)

Но мне надо еще почитать про то, когда все-таки идиоматично возвращать итераторы. Пока не будем усложнять без четкого понимания зачем.

smirnovei commented

2026-03-29 08:18:49 +00:00

(Migrated from git.csit.sgu.ru)

Я бы поспорил на эту тему, т.к. при парсинге одного файла тебе возвращается Result<Table> => у тбея будет Vec<Result<Table>>, а не Vec<Table>. А Vec<Result<Table>> ты точно будешь как-то мапить или фильтровать, чтобы обработать все Result и какой смысл тогда от Vec?

Ограничений на лайфтайм вообще нет и я не вижу откуда они могут здесь вылезти, обычное iter().map по path и все. Ограничения на лайфтайм будут в реализации кеширования парсеров, но оно вылезет и без возвращения итератора.

Единственный минус, который я пока что вижу - вариант с итератором ругается на todo! вместо возврата итератора, но это скорее повод открыть issue в репозитории rust-lang)

Я бы поспорил на эту тему, т.к. при парсинге одного файла тебе возвращается `Result<Table>` => у тбея будет `Vec<Result<Table>>`, а не `Vec<Table>`. А `Vec<Result<Table>>` ты точно будешь как-то мапить или фильтровать, чтобы обработать все `Result` и какой смысл тогда от `Vec`? Ограничений на лайфтайм вообще нет и я не вижу откуда они могут здесь вылезти, обычное iter().map по path и все. Ограничения на лайфтайм будут в реализации кеширования парсеров, но оно вылезет и без возвращения итератора. Единственный минус, который я пока что вижу - вариант с итератором ругается на todo! вместо возврата итератора, но это скорее повод открыть issue в репозитории rust-lang)

smirnovei commented

2026-03-29 08:25:55 +00:00

(Migrated from git.csit.sgu.ru)

Из кеширования парсеров в try_parse_file вылезает, то что тебе придется убирать ограничения с трейта Parser и переносить их в метод трейта, а точнее делать через dyn. Поэтому либо кеширование парсеров - оверхед, либо указание вешать ограничения на Parser заставит переписывать трейт, когда человек дойдет до кеширования парсеров и не сможет сделать его именно с таким трейтом.

Думаю стоит показать это в четверг на моей реализации. + там нет ничего мега сложного и на лекции по трейтам это объяснялось.

Из кеширования парсеров в `try_parse_file` вылезает, то что тебе придется убирать ограничения с трейта `Parser` и переносить их в метод трейта, а точнее делать через dyn. Поэтому либо кеширование парсеров - оверхед, либо указание вешать ограничения на `Parser` заставит переписывать трейт, когда человек дойдет до кеширования парсеров и не сможет сделать его именно с таким трейтом. Думаю стоит показать это в четверг на моей реализации. + там нет ничего мега сложного и на лекции по трейтам это объяснялось.

smirnovei commented

2026-03-29 08:28:01 +00:00

(Migrated from git.csit.sgu.ru)

Да, ок. Просто в начале сема оговаривалось, что лучше не плодить много MR-ов и пихать все в один. Потом вынесу в отдельный, когда будет время.

rydanovns commented

2026-03-29 09:57:12 +00:00

(Migrated from git.csit.sgu.ru)

А что за кэширование парсеров? зачем?

rydanovns commented

2026-03-29 10:11:56 +00:00

(Migrated from git.csit.sgu.ru)

Первое: про лайфтаймы. Сейчас ты принимаешь paths: &[P] по ссылке, неявно там &'a [P]. Когда ты делаешь paths.iter().map(...), ты создаешь итератор, который внутри себя захватил элементы paths по ссылке. Какой у нее будет лайфтайм? У нее, логично, будет 'a, поэтому у тебя по факту возвращаемый тип -> impl Iterator<Item = Result<Table, Errors>> + 'a. Это можно проверить так:

    #[test]
    fn outlive() {
        fn main() {
            let iter;
            {
                let paths = vec!["a.csv", "b.tsv"];
                iter = try_parse_files(&paths);
            }
            let _ = iter.collect::<Vec<_>>();
        }
    }

Так что с лайфтаймами нюансы есть.

Одно популярное использование итераторов — это когда ты хочешь без лишних аллокаций получить какое-то удобное представление над данными (например, разбить на подстроки по пробелу и т.д). Но у тебя сейчас внутри итератора владеющие структуры, поэтому этот аргумент не валиден. И даже если ты сделаешь &str вместо String он тоже будет невалиден, потому что у тебя полученные строки в таблице не являются производными от входных данных. Можно мыслить так. Ты не трансформируешь напрямую входной поток, ты используешь данные на входе и дальше используешь внешний эффект (чтение данных).

Еще итераторы хороши когда ты хочешь написать программную абстракцию, которая хочет производить данные, но не хочет держать разом все данные в памяти. Если бы ты написал функцию, которая возвращает по одной конкретной строке из файла — я бы прикол понял. Возврат парсинга по одному файлу оправдать сложнее (лично мне), хотя тут я соглашусь, что все-таки логика тут присутствует.

Ну и кстати если ты возвращаешь в виде итератора чтение файлов, ты делаешь ленивую логику, в которой I/O операции будут размазаны на вызовы next(), что для вызывающего неочевидно и может быть некрасиво. В общем и целом, такое применение итераторов может создать студентам ложное ощущение, что так правильно, хотя на деле неочевидно зачем они нужны.

Короче, не все тут так просто.

Первое: про лайфтаймы. Сейчас ты принимаешь paths: &[P] по ссылке, неявно там &'a [P]. Когда ты делаешь `paths.iter().map(...)`, ты создаешь итератор, который внутри себя захватил элементы `paths` по ссылке. Какой у нее будет лайфтайм? У нее, логично, будет 'a, поэтому у тебя по факту возвращаемый тип `-> impl Iterator<Item = Result<Table, Errors>> + 'a`. Это можно проверить так: ``` #[test] fn outlive() { fn main() { let iter; { let paths = vec!["a.csv", "b.tsv"]; iter = try_parse_files(&paths); } let _ = iter.collect::<Vec<_>>(); } } ``` Так что с лайфтаймами нюансы есть. Одно популярное использование итераторов — это когда ты хочешь без лишних аллокаций получить какое-то удобное представление над данными (например, разбить на подстроки по пробелу и т.д). Но у тебя сейчас внутри итератора владеющие структуры, поэтому этот аргумент не валиден. И даже если ты сделаешь &str вместо String он тоже будет невалиден, потому что у тебя полученные строки в таблице не являются производными от входных данных. Можно мыслить так. Ты не трансформируешь напрямую входной поток, ты используешь данные на входе и дальше используешь внешний эффект (чтение данных). Еще итераторы хороши когда ты хочешь написать программную абстракцию, которая хочет производить данные, но не хочет держать разом все данные в памяти. Если бы ты написал функцию, которая возвращает по одной конкретной строке из файла — я бы прикол понял. Возврат парсинга по одному файлу оправдать сложнее (лично мне), хотя тут я соглашусь, что все-таки логика тут присутствует. Ну и кстати если ты возвращаешь в виде итератора чтение файлов, ты делаешь ленивую логику, в которой I/O операции будут размазаны на вызовы next(), что для вызывающего неочевидно и может быть некрасиво. В общем и целом, такое применение итераторов может создать студентам ложное ощущение, что так правильно, хотя на деле неочевидно зачем они нужны. Короче, не все тут так просто.

rydanovns commented

2026-03-29 10:21:06 +00:00

(Migrated from git.csit.sgu.ru)

Не помню, чтобы я говорил, что много MR — плохо. Я, наверное, говорил, что неточности и опечатки можно скопом вместе в 1 MR собирать, но переписывание typst в эту рамку вряд ли влезает.

smirnovei commented

2026-03-29 10:33:32 +00:00

(Migrated from git.csit.sgu.ru)

Ну ты же сам говорил, что ты хочешь, чтобы у тебя не создавался парсер на каждый файл, а переиспользовался. Мы же чисто ради этого тогда про фабрику говорить начали.

rydanovns commented

2026-03-29 10:49:53 +00:00

(Migrated from git.csit.sgu.ru)

Ты прав, было, но я не очень осознавал, что у нас у парсера похоже нет своего внутреннего состояния. А раз нет его, то структура весит 0 и непонятно что кэшировать :)

smirnovei commented

2026-03-29 10:50:27 +00:00

(Migrated from git.csit.sgu.ru)

С последним пока не до конца переварил, но соглашусь, наверное это хороший аргумент, потом поправлю, но все таки на Vec<Result<Table>>, а не Vec<Table>.

С лайфтаймом: я понимаю, что в твоем примере это может привести к проблемам, но в мой голове это не является проблемой, т.к. я не могу представить ситуацию где мне итератор нужен дольше чем входные данные.

С последним пока не до конца переварил, но соглашусь, наверное это хороший аргумент, потом поправлю, но все таки на `Vec<Result<Table>>`, а не `Vec<Table>`. С лайфтаймом: я понимаю, что в твоем примере это может привести к проблемам, но в мой голове это не является проблемой, т.к. я не могу представить ситуацию где мне итератор нужен дольше чем входные данные.

grigorevde commented

2026-04-25 20:42:24 +00:00

(Migrated from git.csit.sgu.ru)

restored source branch github/fork/aragami3070/aragami3070/fix-4-traits-parser

restored source branch `github/fork/aragami3070/aragami3070/fix-4-traits-parser`

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin github/fork/aragami3070/aragami3070/fix-4-traits-parser:github/fork/aragami3070/aragami3070/fix-4-traits-parser

git switch github/fork/aragami3070/aragami3070/fix-4-traits-parser

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main

git merge --no-ff github/fork/aragami3070/aragami3070/fix-4-traits-parser

git switch github/fork/aragami3070/aragami3070/fix-4-traits-parser

git rebase main

git switch main

git merge --ff-only github/fork/aragami3070/aragami3070/fix-4-traits-parser

git switch github/fork/aragami3070/aragami3070/fix-4-traits-parser

git rebase main

git switch main

git merge --no-ff github/fork/aragami3070/aragami3070/fix-4-traits-parser

git switch main

git merge --squash github/fork/aragami3070/aragami3070/fix-4-traits-parser

git switch main

git merge --ff-only github/fork/aragami3070/aragami3070/fix-4-traits-parser

git switch main

git merge github/fork/aragami3070/aragami3070/fix-4-traits-parser

git push origin main

Rows
Columns

fix(4.traits): реворк задания по парсеру #15

Checkout

Merge