add 002-my-vcs #2
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yurii-litvinov
requested changes
Apr 16, 2017
Collaborator
yurii-litvinov
left a comment
yurii-litvinov
left a comment
There was a problem hiding this comment.
- неплохо бы создавать какую-нибудь ветку по умолчанию, чтобы не вынуждать вручную создавать ветку перед первым коммитом
log -bпишет current branch: null всегда- создал бранч, добавил туда файл, закоммитил, переключился на первый бранч, переключился обратно ---
Exception in thread "main" java.lang.NullPointerException
at java.nio.file.Files.provider(Files.java:97)
at java.nio.file.Files.createDirectory(Files.java:674)
at java.nio.file.Files.createAndCheckIsDirectory(Files.java:781)
at java.nio.file.Files.createDirectories(Files.java:727)
at com.maxim.vcs_impl.VcsImpl.checkoutCommit(VcsImpl.java:101)
at com.maxim.vcs_impl.VcsImpl.checkoutBranch(VcsImpl.java:120)
at com.maxim.Main$4.execute(Main.java:60)
at com.maxim.Main.run(Main.java:140)
at com.maxim.Main.main(Main.java:132)
- нельзя удалить ветку?
- есть подозрение, что log -c показывает все коммиты вообще, а не коммиты текущей ветки
| package com.maxim; | ||
|
|
||
| import com.google.common.collect.ImmutableList; | ||
| import com.google.common.collect.ImmutableSet; |
Collaborator
There was a problem hiding this comment.
IDEA говорит, что этот класс тут не нужен
| } | ||
|
|
||
| public void run() { | ||
| List<Command> ok_commands = commands.stream().filter(command -> command.check(args)).collect(Collectors.toList()); |
Collaborator
There was a problem hiding this comment.
ok_commands как-то не по стайлгайду. camelCase же (https://docs.oracle.com/javase/tutorial/java/nutsandbolts/variables.html)
|
|
||
| private abstract static class Command { | ||
| private final ImmutableList<String> args_names; | ||
| private final int prefix_len; |
Collaborator
There was a problem hiding this comment.
Тут тоже все поля не по стайлгайду названы
| } catch (ClassNotFoundException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| throw new RuntimeException(); |
Collaborator
There was a problem hiding this comment.
Так мы теряем информацию о причине исключения, лучше e обернуть
| Files.createDirectories(commits_dir_path); | ||
| Files.createDirectories(blobs_dir_path); | ||
| Files.createDirectories(branches_dir_path); | ||
| Files.createDirectories(branches_dir_path); |
| readIndex(); | ||
|
|
||
| if (index.branch == null) { | ||
| throw new IOException("Please, checkout branch or create branch"); |
Collaborator
There was a problem hiding this comment.
Лучше свои исключения кидать, иначе не отличить ошибку на уровне библиотеки от ошибки на уровне Вашего кода
| vcs.createBranch("branch"); | ||
|
|
||
| Callable<List<Path>> getAdded = () -> { | ||
| Map < Path, String > cur_status = vcs.status(); |
Collaborator
There was a problem hiding this comment.
Необычная расстановка пробелов
| @Test | ||
| public void logTest() throws IOException { | ||
| final Vcs vcs = new VcsImpl(Paths.get(temporaryFolder.getRoot().getPath())); | ||
| List<Path> paths = initFolder(); |
Collaborator
There was a problem hiding this comment.
Кажется, не используется
| this.commit_id = branch.commit_id; | ||
| } | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
Суперумный репозиторий, против которого я выступал на паре. Нельзя ли часть функциональности перенести в vcs_objects или в новые классы?
yurii-litvinov
requested changes
May 8, 2017
Collaborator
yurii-litvinov
left a comment
yurii-litvinov
left a comment
There was a problem hiding this comment.
По поводу продвинутой VCS:
- status показывает статус файлов из .vcs, а не должен
- файлы, которые commited, лучше в status вообще не показывать, иначе для большого проекта мы никогда содержательную информацию в выдаче не найдём
- если просто удалить закоммиченный файл с диска, status его вообще не покажет (а должен говорить что-то про то, что он missing)
- кстати, reset на удалённый таким образом файл ругается "no such committed file", но он же закоммичен, просто его в рабочей копии нет
- если на изменённый файл сделать add, то status всё равно показывает его как modified (а должен как staged или что-то такое)
- команда clean снесла вообще весь репозиторий, оставив только пустые папки. Кстати, по-хорошему, пустые папки лучше удалять (гит по умолчанию так не делает, но там опция есть)
Ещё по условию требовалось добавить логирование и mock-объекты.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.