Skip to content

Conversation

@vasyoid
Copy link

@vasyoid vasyoid commented Feb 16, 2019

В целом архитектура порадовала.

Из плюсов:

  • Для добавления новой команды достаточно дописать одну строчку в имеющийся класс и создать один новый.
  • Каждая команда уже "из коробки" знает свои stdin stdout, в том числе известно, вызвалась ли команда первой в пайплайне или в середине.

Из минусов:

  • Изначально не была продумана возможность изменения текузей директории, из-за чего пришлось исправлять все уже написанные команды. Впрочем, с этим столкнулись почти все, кто писали на jvm-языках, и тут есть вопрос к разработчикам стандарта java.

@ArtyomLobanov
Copy link

Я не совсем понял, а что вы от них хотите?

тут есть вопрос к разработчикам стандарта java.

File(path).canonicalFile
} else {
File(workingDirectory, path).canonicalFile
}

Choose a reason for hiding this comment

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

Эта логика уже реализована в Path.resolve

}

private fun isAbsolutePath(path: String): Boolean {
return File.listRoots().fold(false) { res, file -> res || path.startsWith(file.canonicalPath) }

Choose a reason for hiding this comment

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

И это тоже не нужно, если работать с путями через Path

*/
override fun execute(output: PipedWriter) {
if (arguments.size > 1) {
output.write("cd: too many arguments")

Choose a reason for hiding this comment

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

Не надо смешивать поток вывода команд и сообщения об ошибках. Бросайте исключения

output.write("cd: too many arguments")
return
}
if (!env.changeDirectory(arguments.getOrElse(0) { "./" })) {

Choose a reason for hiding this comment

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

Не кроссплатформенно.

>pwd
C:\practice\kuporosov\SoftwareDesign\bash
>cd
>pwd
C:\practice\kuporosov\SoftwareDesign\bash

Copy link
Author

@vasyoid vasyoid Mar 21, 2019

Choose a reason for hiding this comment

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

Проблема в использовании '/'? Но вроде это ничего не ломает.
У меня, кажется, одинаковое поведение на windows и ubuntu.
5NPyYFjaSdg
2019-03-21 11_37_45-bash  C__Users_Vasily Kuporosov_SoftwareDesign_bash  -  _src_main_kotlin_hse_n

Choose a reason for hiding this comment

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

А, тогда кроссплатформенно не работает)
cd без аргументов должна менять рабочую директорию на домашнюю
Ожидалось такое поведение:

>pwd
C:\practice\kuporosov\SoftwareDesign\bash
>cd
>pwd
C:\Users\Артём

Copy link
Author

Choose a reason for hiding this comment

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

То есть надо обеспечить разное поведение на разных системах?

Choose a reason for hiding this comment

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

Нет, одинаковое - переход в домашнюю папку

@vasyoid
Copy link
Author

vasyoid commented Mar 20, 2019

Я не совсем понял, а что вы от них хотите?

тут есть вопрос к разработчикам стандарта java.

Насколько я понял из данного обсуждения, стандарт java не предусматривает обязательной поддержки механизма, позволяющего поменять рабочую директорию.

*/
override fun execute(output: PipedWriter) {
if (arguments.size > 1) {
output.write("ls: too many arguments")

Choose a reason for hiding this comment

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

В ls ещё не исправили вывод сообщений об ошибках

@ArtyomLobanov
Copy link

Можно было бы отдельный класс ошибки создать, а не использовать RuntimeException.
В остальном - ок. @yurii-litvinov

Copy link

@yurii-litvinov yurii-litvinov left a comment

Choose a reason for hiding this comment

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

Последовательность команд

cd ..
ls
.git
.gitignore
bash
LICENSE
cat LICENSE

не приводит к ожидаемому результату, будто cat не знает про смену папки.

Но это единственный замеченный недостаток, так что не буду просить исправить, зачтена.

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.

3 participants