Skip to content

ls & cd#4

Open
G74v3Dw31137 wants to merge 2 commits into
tretikoff:grep-commandfrom
G74v3Dw31137:grep-command
Open

ls & cd#4
G74v3Dw31137 wants to merge 2 commits into
tretikoff:grep-commandfrom
G74v3Dw31137:grep-command

Conversation

@G74v3Dw31137
Copy link
Copy Markdown

No description provided.

@G74v3Dw31137
Copy link
Copy Markdown
Author

Удобным было то, что команды наследуются от общего интерфейса, их было легко реализовать. Тесты тоже наследуются от базового, даже создан тестовый файл. Сильно неудобным было то, что нельзя поменять рабочую директорию и получается, что при запуске cd мы на самом деле просто сохраняем базовый путь, и мне пришлось поменять класс FileStream, а так же добавить этот базовый путь в exec в ExternalCommand. Также я заметил, что реализация ExternalCommand не умеет принимать аргументы, что мне показалось странным.

Copy link
Copy Markdown

@ottergottaott ottergottaott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В целом, ок, поправьте мелочи, пока 9 баллов

errorStream.writeLine("not a directory: \"$path\"")
return 3
}
System.setProperty("user.dir", file.getAbsolutePath())
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file.agbsolutePath, это же котлин

override fun execute(): Int {
logger.finest("Running cd with arguments $args")
if (args.isEmpty()) {
val path = System.getProperty("user.home")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вообще кажется плохой идеей так менять рабочую директорию, т.к. потом элементарно сложнее тестировать. Если это переменная окружения, то окружение можно замокать, а System.getProperty уже вряд ли.

Сейчас не видно проблему, т.к. тесты не юнит, а все-таки интеграционные, но если бы они были юнит-тестами, то быстро бы всплыло, что от запуска одного теста к запуску другого приходилось бы танцевать с бубном, чтобы тесты правильно работали

parent = System.getProperty("user.dir")
}
val file = File(parent, path).getAbsoluteFile()
val files = file.list()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Чего-то не понял почему мы сначала пытаемся list взять, а только потом проверяем существует ли вообще файл

val path = System.getProperty("user.dir")
return lsPath(path)
} else if (args.count() == 1) {
return lsPath(args.get(0))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`args[0]``

@ottergottaott
Copy link
Copy Markdown

9 баллов окончательные

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants