[#22] Add contacts #96

willbasky · 2018-09-06T09:16:59Z

Resolves #22

This change is

cblp

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @willbasky)

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

    -> Maybe Day
    -> Maybe Tracked
    -> m Note

?

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

    noteText     <- rgaFromText text
    noteStart    <- LWW.initialize start
    noteEnd      <- LWW.initialize end

?

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

    }

newtype Contact = ContactShow (Maybe ContactCommand)

Show?

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

data Contact = Contact
    { contactStatus :: LWW Status

Контакт не может быть вики.

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

instance Semilattice Note

instance Semilattice Contact

Лучше сгруппировать инстансы по типам, а не по классам.

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

    , csTotal :: Natural
    }
    deriving (Eq, Show)

Объедини в один тип с параметром, потому что всё одинаковое. Sample NoteView, Sample ContactView

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

    { cvId     = contactId
    , cvStatus = LWW.query contactStatus
    , cvName   = Text.pack $ RGA.toString contactName

rgaToText

willbasky · 2018-09-06T09:51:34Z

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

Previously, cblp (Yuriy Syrovetskiy) wrote…

?

?

willbasky · 2018-09-06T09:52:10Z

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

Previously, willbasky (Vladislav Sabanov) wrote…

?

Длинно было.

willbasky · 2018-09-06T09:53:58Z

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

ContactShow

показывает все контакты

willbasky · 2018-09-06T09:55:46Z

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

Previously, cblp (Yuriy Syrovetskiy) wrote…

Контакт не может быть вики.

А что значит тут вики?

cblp

Reviewed 5 of 5 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @cblp and @willbasky)

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

Previously, willbasky (Vladislav Sabanov) wrote…

ContactShow
показывает все контакты

Удалять — это тоже показывать?

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

Previously, willbasky (Vladislav Sabanov) wrote…

А что значит тут вики?

У типа Status есть значение Wiki. Оно имеет смысл только для заметок, но не для контактов.

cblp

Reviewed 6 of 6 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @willbasky)

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

    }

newtype Contact = Contact (Maybe ContactCommand)

Бессмысленная обёртка. Можно в CmdContact класть сразу Maybe ContactCommand

cblp

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @willbasky)

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

import           FF.Types.Internal (noteJsonOptions)

data StatusNote = Active | Archived | Deleted | Wiki

Как-то некрасивенько. Added и Removed имеют абсолютно такой же смысл, как Active и Deleted. И Archived тоже полезно бы иметь.

Предлагаю такое:

data Status = Active | Archived | Deleted
data Wiki = Wiki
...
, contactStatus :: Status
...
, noteStatus :: Either Wiki Status

cblp

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @willbasky)

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

Previously, cblp (Yuriy Syrovetskiy) wrote…

Как-то некрасивенько. Added и Removed имеют абсолютно такой же смысл, как Active и Deleted. И Archived тоже полезно бы иметь.

Предлагаю такое:
data Status = Active | Archived | Deleted
data Wiki = Wiki
...
, contactStatus :: Status
...
, noteStatus :: Either Wiki Status

всё под LWW, конечно

cblp

Reviewed 5 of 5 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

cblp

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

ff/Main.hs, line 179 at r4 (raw file):

cmdContact :: Maybe Contact -> Storage ()
cmdContact mCommand = case mCommand of

\case

cblp

Reviewed 4 of 4 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

cblp

Reviewed 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved

…acts

cblp

Reviewed 5 of 5 files at r7.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @willbasky)

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

      where
        st = case ns of
            TaskStatus s -> show s

s/show/toJSON/

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

        st = case ns of
            TaskStatus s -> show s
            _ -> show Wiki

вместо _ лучше явно написать Wiki для устойчивости к будущим изменениям

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

instance FromJSON NoteStatus where
    parseJSON = withText "NoteStatus" $ \x -> do
        let s = case x of

можно не вводить s, а сразу написать return $ case ...

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

                "Archived" -> TaskStatus Archived
                "Deleted"  -> TaskStatus Deleted
                _          -> error "Not NoteStatus type"

s/error/fail/

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

                "Deleted"  -> TaskStatus Deleted
                _          -> error "Not NoteStatus type"
        return s

s/return/pure/

ff-test/test/ArbitraryOrphans.hs, line 25 at r7 (raw file):

instance Arbitrary NoteStatus where
    arbitrary = TaskStatus <$> arbitrary

такой генератор никогда не сгенерит Wiki

cblp

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

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

                "Active"   -> TaskStatus Active
                "Archived" -> TaskStatus Archived
                "Deleted"  -> TaskStatus Deleted

эти 3 строчки можно сократить до TaskStatus <$> parseJSON v

willbasky · 2018-09-12T03:16:26Z

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

Previously, cblp (Yuriy Syrovetskiy) wrote…

s/show/toJSON/

Что означает такой комментарий?

willbasky · 2018-09-12T03:22:53Z

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

TaskStatus <$> parseJSON v

Тогда будет ошибка

Couldn't match expected type ‘NoteStatus’
              with actual type ‘aeson-1.3.1.1:Data.Aeson.Types.Internal.Parser
                                  NoteStatus’

willbasky · 2018-09-12T03:49:55Z

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

v

Ок, даже если заменить, то откуда брать v обощенную и именно содержащую нужные строки, а не любые? Я сделал так сейчас

instance FromJSON NoteStatus where
    parseJSON = withText "NoteStatus" $ \case
        "Wiki"     -> parseJSON "Wiki"
        "Active"   -> TaskStatus <$> parseJSON "Active"
        "Archived" -> TaskStatus <$> parseJSON "Archived"
        "Deleted"  -> TaskStatus <$> parseJSON "Deleted"
        _          -> fail "Not NoteStatus type"

cblp

Reviewable status: 8 of 10 files reviewed, 6 unresolved discussions (waiting on @cblp and @willbasky)

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

Previously, willbasky (Vladislav Sabanov) wrote…

Что означает такой комментарий?

заменить одно на другое

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

    toJSON = \case
        TaskStatus s -> toJSON s
        Wiki -> toJSON Wiki

зависнет. вот здесь как раз настоящую реализацию надо написать

cblp

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

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

Previously, willbasky (Vladislav Sabanov) wrote…

v

Ок, даже если заменить, то откуда брать v обощенную и именно содержащую нужные строки, а не любые? Я сделал так сейчас
instance FromJSON NoteStatus where
    parseJSON = withText "NoteStatus" $ \case
        "Wiki"     -> parseJSON "Wiki"
        "Active"   -> TaskStatus <$> parseJSON "Active"
        "Archived" -> TaskStatus <$> parseJSON "Archived"
        "Deleted"  -> TaskStatus <$> parseJSON "Deleted"
        _          -> fail "Not NoteStatus type"

попробуй parseJSON v = TaskStatus <$> parseJSON v и подумай, куда здесь вставить Wiki

cblp

Reviewed 2 of 2 files at r8.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @willbasky)

ff-test/test/ArbitraryOrphans.hs, line 25 at r8 (raw file):

instance Arbitrary NoteStatus where
    arbitrary = oneof [TaskStatus <$> arbitrary, arbitrary]

arbitrary @NoteStatus вызывает arbitrary @Status, а потом arbitrary @NoteStatus, то есть себя. зависнет

willbasky · 2018-09-12T11:58:27Z

ff-test/test/ArbitraryOrphans.hs, line 25 at r7 (raw file):

Previously, cblp (Yuriy Syrovetskiy) wrote…

такой генератор никогда не сгенерит Wiki

А каким образом складывать arbitrary для типов, у которых значения суммы?
Подобные вещи не проканывают, тест виснит:

arbitrary = elements [TaskStatus Active, TaskStatus Archived, TaskStatus Deleted, Wiki]

willbasky · 2018-09-12T11:59:43Z

ff-test/test/ArbitraryOrphans.hs, line 25 at r7 (raw file):

Previously, willbasky (Vladislav Sabanov) wrote…

А каким образом складывать arbitrary для типов, у которых значения суммы?
Подобные вещи не проканывают, тест виснит:
arbitrary = elements [TaskStatus Active, TaskStatus Archived, TaskStatus Deleted, Wiki]

так тоже виснит

arbitrary = oneof [TaskStatus <$> arbitrary, pure Wiki]

cblp

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @cblp and @willbasky)

ff-test/test/ArbitraryOrphans.hs, line 25 at r7 (raw file):

Previously, willbasky (Vladislav Sabanov) wrote…

так тоже виснит
arbitrary = oneof [TaskStatus <$> arbitrary, pure Wiki]

Оба варианта правильные, зависает в другом месте.

cblp

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @willbasky)

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

    deriving (Eq, Show)

type SampleContact = Sample ContactView

ContactSample

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

type SampleContact = Sample ContactView

type SampleNote = Sample NoteView

NoteSample

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

taskMode :: Day -> NoteView -> TaskMode
taskMode today NoteView { start, end } = case end of

лишние изменения в форматировании. прогнал brittany?

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

taskMode today NoteView { start, end } = case end of
    Nothing | start <= today -> Actual
            | otherwise      -> starting start today

лишние изменения в форматировании. прогнал brittany?

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

            , ["| end"      <+> pshow @Day e | Just e <- [end]]
            ]
        ++  [ "| tracking"  <+> strictText trackedUrl

лишние изменения в форматировании. прогнал brittany?

ff-test/test/ArbitraryOrphans.hs, line 4 at r8 (raw file):

module ArbitraryOrphans
    ()

лишние изменения в форматировании. прогнал brittany?

cblp

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @cblp and @willbasky)

ff-test/test/ArbitraryOrphans.hs, line 25 at r7 (raw file):

Previously, cblp (Yuriy Syrovetskiy) wrote…

Оба варианта правильные, зависает в другом месте.

я даже написал комментарий о зависании в том месте

willbasky · 2018-09-12T13:23:51Z

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

Previously, cblp (Yuriy Syrovetskiy) wrote…

лишние изменения в форматировании. прогнал brittany?

нее, это stylish-haskell шалил.

willbasky · 2018-09-12T13:25:04Z

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

Previously, cblp (Yuriy Syrovetskiy) wrote…

лишние изменения в форматировании. прогнал brittany?

не вижу, где тут?

cblp

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @willbasky and @cblp)

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

Previously, willbasky (Vladislav Sabanov) wrote…

не вижу, где тут?

https://github.com/ff-notes/ff/pull/96/files?utf8=✓&diff=unified#diff-47b8ca6074ea5df4616278d21d47162fL97

willbasky · 2018-09-12T13:50:23Z

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

Previously, cblp (Yuriy Syrovetskiy) wrote…

зависнет. вот здесь как раз настоящую реализацию надо написать

По какой причине здесь зависает?

cblp

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @willbasky and @cblp)

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

Previously, willbasky (Vladislav Sabanov) wrote…

По какой причине здесь зависает?

toJSON Wiki вычисляется через toJSON Wiki, то есть через себя

cblp

Reviewed 6 of 6 files at r9.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @willbasky)

ff-core/lib/FF/Types.hs, line 52 at r9 (raw file):

instance ToJSON NoteStatus where
    toJSON (TaskStatus a) = toJSON a
    toJSON _ = String "Wiki"

Следует явно указать Wiki в аргументе для определённости и устойчивости к будущим изменениям (future-proof)

ff-core/lib/FF/Types.hs, line 57 at r9 (raw file):

    parseJSON v = case v of
        "Wiki" -> pure Wiki
        _ -> TaskStatus <$> parseJSON v

Идеально!

cblp

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

ff-core/lib/FF/Types.hs, line 52 at r9 (raw file):

Previously, cblp (Yuriy Syrovetskiy) wrote…

Следует явно указать Wiki в аргументе для определённости и устойчивости к будущим изменениям (future-proof)

Ещё мне больше нравится \case, но это вкусовщина, можно и так оставить.

cblp

Reviewed 1 of 1 files at r10.
Reviewable status: complete! all files reviewed, all discussions resolved

[#22] Add contacts

906b677

cblp requested changes Sep 6, 2018

View reviewed changes

Refactor style

e6dbd67

cblp requested changes Sep 6, 2018

View reviewed changes

Fix status

edf95cd

cblp requested changes Sep 6, 2018

View reviewed changes

willbasky added 2 commits September 7, 2018 11:55

Fix Options.Contact

91ec371

Refactor Status type

d8e2345

cblp approved these changes Sep 7, 2018

View reviewed changes

cblp requested changes Sep 7, 2018

View reviewed changes

Fix tests

143558f

cblp approved these changes Sep 7, 2018

View reviewed changes

Fix instance

01a212b

cblp approved these changes Sep 7, 2018

View reviewed changes

willbasky added 2 commits September 7, 2018 15:56

Merge remote-tracking branch 'upstream/master' into willbasky/22-cont…

41ede2c

…acts

Fix status types and add json instances

8809732

cblp requested changes Sep 11, 2018

View reviewed changes

Refactor instance and arbitrary

3f2a6a6

cblp requested changes Sep 12, 2018

View reviewed changes

Fix instanse for NoteStatus

ce0ce24

cblp requested changes Sep 12, 2018

View reviewed changes

Refactor style

ec3551e

cblp approved these changes Sep 12, 2018

View reviewed changes

cblp merged commit 6059167 into ff-notes:master Sep 12, 2018

willbasky deleted the willbasky/22-contacts branch September 12, 2018 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#22] Add contacts #96

[#22] Add contacts #96

willbasky commented Sep 6, 2018 •

edited by cblp

Loading

cblp left a comment

willbasky commented Sep 6, 2018

willbasky commented Sep 6, 2018

willbasky commented Sep 6, 2018

willbasky commented Sep 6, 2018

cblp left a comment

cblp left a comment

cblp left a comment

cblp left a comment

cblp left a comment

cblp left a comment

cblp left a comment

cblp left a comment

cblp left a comment

cblp left a comment

willbasky commented Sep 12, 2018

willbasky commented Sep 12, 2018

willbasky commented Sep 12, 2018

cblp left a comment

cblp left a comment

cblp left a comment

willbasky commented Sep 12, 2018

willbasky commented Sep 12, 2018

cblp left a comment

cblp left a comment

cblp left a comment

willbasky commented Sep 12, 2018

willbasky commented Sep 12, 2018

cblp left a comment

willbasky commented Sep 12, 2018

cblp left a comment

cblp left a comment

cblp left a comment

cblp left a comment

[#22] Add contacts #96

[#22] Add contacts #96

Conversation

willbasky commented Sep 6, 2018 • edited by cblp Loading

cblp left a comment

Choose a reason for hiding this comment

willbasky commented Sep 6, 2018

willbasky commented Sep 6, 2018

willbasky commented Sep 6, 2018

willbasky commented Sep 6, 2018

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

willbasky commented Sep 12, 2018

willbasky commented Sep 12, 2018

willbasky commented Sep 12, 2018

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

willbasky commented Sep 12, 2018

willbasky commented Sep 12, 2018

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

willbasky commented Sep 12, 2018

willbasky commented Sep 12, 2018

cblp left a comment

Choose a reason for hiding this comment

willbasky commented Sep 12, 2018

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

cblp left a comment

Choose a reason for hiding this comment

willbasky commented Sep 6, 2018 •

edited by cblp

Loading