Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ff new --vcs (#171) #197

Merged
merged 14 commits into from Oct 20, 2019
Merged

ff new --vcs (#171) #197

merged 14 commits into from Oct 20, 2019

Conversation

@willbasky
Copy link
Contributor

willbasky commented Oct 9, 2019

Resolves #171


This change is Reviewable

@willbasky willbasky self-assigned this Oct 9, 2019
@willbasky willbasky requested a review from cblp Oct 9, 2019
Copy link
Member

cblp left a comment

Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @cblp and @willbasky)


ff-core/lib/FF.hs, line 635 at r1 (raw file):

      isDirVcsGit <- doesDirectoryExist (dir </> ".git")
      isDirVcsFF <- doesDirectoryExist (dir </> ".ff")
      if isDirVcsGit && isDirVcsFF

Но так .ff никогда не появится. Нужен способ создать .ff.

@willbasky

This comment has been minimized.

Copy link
Contributor Author

willbasky commented Oct 10, 2019


ff-core/lib/FF.hs, line 635 at r1 (raw file):

Previously, cblp (Yuriy Syrovetskiy) wrote…

Но так .ff никогда не появится. Нужен способ создать .ff.

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

Copy link
Member

cblp left a comment

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @willbasky)


ff-core/lib/FF.hs, line 635 at r1 (raw file):

Previously, willbasky (Vladislav Sabanov) wrote…

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

Нужен способ не произвольного, но умышленного создания .ff. ff config — хороший вариант. Я ещё думал про ff new с опцией, например,--git.

willbasky added 3 commits Oct 11, 2019
@willbasky willbasky changed the title Fix getDataDir (#171) FF new --vcs (#171) Oct 15, 2019
@willbasky willbasky changed the title FF new --vcs (#171) ff new --vcs (#171) Oct 15, 2019
Copy link
Member

cblp left a comment

Reviewed 1 of 7 files at r2.
Reviewable status: 1 of 7 files reviewed, all discussions resolved (waiting on @willbasky)

Copy link
Member

cblp left a comment

Reviewed 1 of 7 files at r2.
Reviewable status: 2 of 7 files reviewed, all discussions resolved (waiting on @willbasky)

Copy link
Member

cblp left a comment

Reviewed 1 of 7 files at r2.
Reviewable status: 3 of 7 files reviewed, all discussions resolved (waiting on @willbasky)

Copy link
Member

cblp left a comment

Reviewable status: 3 of 7 files reviewed, 1 unresolved discussion (waiting on @willbasky)


ff-core/lib/FF.hs, line 652 at r2 (raw file):

noVcs :: String
noVcs = "You set '--vcs', but there are no any directory containing .git repo"

"there is no directory"

Copy link
Member

cblp left a comment

Reviewed 2 of 7 files at r2.
Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @willbasky)


ff-core/lib/FF.hs, line 636 at r2 (raw file):

    findVcs (dir : dirs) = do
      isDirVcsGit <- doesDirectoryExist (dir </> ".git")
      isDirVcsFF <- doesDirectoryExist (dir </> ".ff")

FF не VCS

Copy link
Member

cblp left a comment

Reviewable status: 5 of 7 files reviewed, 3 unresolved discussions (waiting on @willbasky)


ff-core/lib/FF.hs, line 637 at r2 (raw file):

      isDirVcsGit <- doesDirectoryExist (dir </> ".git")
      isDirVcsFF <- doesDirectoryExist (dir </> ".ff")
      case (isDirVcsGit, isDirVcsFF) of

перепиши на if, пожалуйста

Copy link
Member

cblp left a comment

Reviewable status: 5 of 7 files reviewed, 5 unresolved discussions (waiting on @willbasky)


ff-core/lib/FF.hs, line 638 at r2 (raw file):

      isDirVcsFF <- doesDirectoryExist (dir </> ".ff")
      case (isDirVcsGit, isDirVcsFF) of
        (True,True) -> pure $ DataDirectory Nothing (Just $ dir </> ".ff")

два поля с одинаковым типом, лучше использовать record syntax для конструктора: DataDirectory{git = Nothing, ff = Just $ gir </> ".ff"}


ff-core/lib/FF.hs, line 643 at r2 (raw file):

data DataDirectory = DataDirectory
  { git :: Maybe FilePath

Из кода непонятен смысл этих полей.

willbasky added 2 commits Oct 17, 2019
Copy link
Member

cblp left a comment

Reviewed 2 of 4 files at r3.
Reviewable status: 5 of 7 files reviewed, all discussions resolved (waiting on @willbasky)

Copy link
Member

cblp left a comment

Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @willbasky)


ff-core/lib/FF.hs, line 639 at r3 (raw file):

      if isDirVcsGit && isDirFF
        then pure $ DataDirectory {gitPath = Nothing, ffPath = Just $ dir </> ".ff"}
        else if isDirVcsGit && not isDirFF

Если пользователь не хочет VCS, то надо искать первый попавшийся .ff отсюда и вверх или брать из конфига, игнорируя встретившиеся на пути .git. Как я понял, эта штука остановится на .git и из конфига не возьмёт.

@willbasky

This comment has been minimized.

Copy link
Contributor Author

willbasky commented Oct 18, 2019


ff-core/lib/FF.hs, line 639 at r3 (raw file):

Previously, cblp (Yuriy Syrovetskiy) wrote…

Если пользователь не хочет VCS, то надо искать первый попавшийся .ff отсюда и вверх или брать из конфига, игнорируя встретившиеся на пути .git. Как я понял, эта штука остановится на .git и из конфига не возьмёт.

если пользователь вводит ff or ff new text тогда он на .git не остановится и дойдет до .ff или конфига.
То есть на .git не останавливается

Copy link
Member

cblp left a comment

Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @willbasky)


ff-core/lib/FF.hs, line 639 at r3 (raw file):

Previously, willbasky (Vladislav Sabanov) wrote…

если пользователь вводит ff or ff new text тогда он на .git не остановится и дойдет до .ff или конфига.
То есть на .git не останавливается

Я не вижу, где происходит проверка следующего каталога. Я вижу, что при isDirVcsGit в любом случае будет остановка на pure DataDirectory{}.

willbasky added 2 commits Oct 20, 2019
@willbasky willbasky force-pushed the willbasky:willbasky/fix-git-repo-use branch from 8aa5a34 to 9f74b02 Oct 20, 2019
Copy link
Member

cblp left a comment

Reviewable status: 3 of 7 files reviewed, 3 unresolved discussions (waiting on @cblp and @willbasky)


ff-core/lib/FF.hs, line 649 at r4 (raw file):

data DataDirectory = DataDirectory
  { vcsNeed :: Maybe FilePath -- ^ new .ff path next to .git directory when vcs needed

"new .ff path in the root VCS directory"

мы когда-нибудь будем работать не только с гитом


ff-core/lib/FF.hs, line 650 at r4 (raw file):

data DataDirectory = DataDirectory
  { vcsNeed :: Maybe FilePath -- ^ new .ff path next to .git directory when vcs needed
  , vcsNotNeed :: Maybe FilePath -- ^ existing .ff path when vcs not needed

"when VCS is not needed"

Copy link
Member

cblp left a comment

Reviewable status: 3 of 7 files reviewed, 2 unresolved discussions (waiting on @cblp and @willbasky)


ff-core/lib/FF.hs, line 646 at r5 (raw file):

              DataDirectory {vcsRequired = Nothing, vcsNotRequired = Just ffDir}
          | isDirVcsGit && not isDirFF = pure $
              DataDirectory {vcsRequired = Just ffDir, vcsNotRequired = dataDir}

Если существует .ff выше .git, то он будет проигнорирован, а должен быть выбран

Copy link
Member

cblp left a comment

Reviewed 3 of 4 files at r5.
Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @willbasky)

Copy link
Member

cblp left a comment

Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @willbasky)


ff-core/lib/FF/CLI.hs, line 126 at r5 (raw file):

          Just h -> runStorage h $ runCmdAction ui action brief
      (CmdNew Options.New{vcs = True}, Just _) -> fail directoryConflict
      (_, customDir') -> do

вместо джокера лучше написать явно {vcs = False}

@willbasky

This comment has been minimized.

Copy link
Contributor Author

willbasky commented Oct 20, 2019


ff-core/lib/FF/CLI.hs, line 126 at r5 (raw file):

Previously, cblp (Yuriy Syrovetskiy) wrote…

вместо джокера лучше написать явно {vcs = False}

Но джокер здесь означает любые actions а не только CmdNew

Copy link
Member

cblp left a comment

Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @willbasky)


ff-core/lib/FF/CLI.hs, line 126 at r5 (raw file):

Previously, willbasky (Vladislav Sabanov) wrote…

Но джокер здесь означает любые actions а не только CmdNew

ок, оставляем

Copy link
Member

cblp left a comment

Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @willbasky)


ff-core/lib/FF.hs, line 652 at r6 (raw file):

        findFF (dir' : dirs') = do
          isDirFF' <- doesDirectoryExist (dir' </> ".ff")
          if isDirFF' then pure $ Just (dir' </> ".ff") else findFF dirs'

как насчёт пробежаться один раз по списку каталогов в поисках только .git и один раз в поисках только .ff?

Copy link
Member

cblp left a comment

Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @willbasky)


ff-core/lib/FF.hs, line 643 at r7 (raw file):

    findFF (dir : dirs) = do
      isDirFF <- doesDirectoryExist (dir </> ".ff")
      if isDirFF then pure $ Just (dir </> ".ff") else findFF dirs

как насчёт обобщить эти 2 функции в одну?

@willbasky willbasky force-pushed the willbasky:willbasky/fix-git-repo-use branch from 140a75d to 7ec5eb6 Oct 20, 2019
Copy link
Member

cblp left a comment

Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @willbasky)


ff-core/lib/FF.hs, line 641 at r8 (raw file):

      if isPath then pure $ Just (dir </> ".ff") else findPath dirs path
    getDataDirectory ffPathNew ffPath
      | (length <$> ffPathNew) > (length <$> ffPath) = pure $

ой, что-то странное. сравнивать дину путей точно нужно?

Copy link
Member

cblp left a comment

Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @willbasky)


ff-core/lib/FF.hs, line 641 at r8 (raw file):

Previously, cblp (Yuriy Syrovetskiy) wrote…

ой, что-то странное. сравнивать дину путей точно нужно?

всё-таки нужны тесты

@willbasky

This comment has been minimized.

Copy link
Contributor Author

willbasky commented Oct 20, 2019


ff-core/lib/FF.hs, line 641 at r8 (raw file):

Previously, cblp (Yuriy Syrovetskiy) wrote…

всё-таки нужны тесты

это такой способ не обращать внимание на .git которые нашлись выше .ff

@willbasky

This comment has been minimized.

Copy link
Contributor Author

willbasky commented Oct 20, 2019


ff-core/lib/FF.hs, line 641 at r8 (raw file):

Previously, willbasky (Vladislav Sabanov) wrote…

это такой способ не обращать внимание на .git которые нашлись выше .ff

тесты пишу

Copy link
Member

cblp left a comment

Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @willbasky)


ff-core/lib/FF.hs, line 641 at r8 (raw file):

Previously, willbasky (Vladislav Sabanov) wrote…

тесты пишу

но длина dataDir из конфига совсем никак не связана с найденными .git и .ff

@willbasky

This comment has been minimized.

Copy link
Contributor Author

willbasky commented Oct 20, 2019


ff-core/lib/FF.hs, line 641 at r8 (raw file):

Previously, cblp (Yuriy Syrovetskiy) wrote…

но длина dataDir из конфига совсем никак не связана с найденными .git и .ff

об этом я не подумал

@willbasky willbasky force-pushed the willbasky:willbasky/fix-git-repo-use branch from 1de21e3 to 50dcb43 Oct 20, 2019
@cblp
cblp approved these changes Oct 20, 2019
Copy link
Member

cblp left a comment

Reviewed 3 of 7 files at r9.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@cblp cblp merged commit 7126749 into ff-notes:master Oct 20, 2019
2 checks passed
2 checks passed
code-review/reviewable 3 files reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@willbasky willbasky deleted the willbasky:willbasky/fix-git-repo-use branch Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.