Closed themaxhero closed 6 years ago
Reviewed 2 of 2 files at r1. Review status: all files reviewed at latest revision, 1 unresolved discussion.
src/Decoders/Account.elm, line 34 at r1 (raw file):
|> hardcoded model.notifications |> hardcoded model.signOut |> mainframe
delete the dummies
Comments from Reviewable
src/Decoders/Account.elm, line 34 at r1 (raw file):
delete the dummies
I can't delete the dummies before the bootstrap been totally completed.
Comments from Reviewable
Reviewed 20 of 20 files at r2. Review status: all files reviewed at latest revision, 9 unresolved discussions.
src/Apps/BounceManager/View.elm, line 220 at r2 (raw file):
Just ForSpinner -> modalOk (Just "Think that i'm a Spinner")
"You spin me right round, baby Right round like a record, baby Right round round round" :musical_note: :laughing:
src/Apps/VirusPanel/Update.elm, line 216 at r2 (raw file):
config.activeGatewayCId folder k v ( acu, found ) =
folder k v acu =
if not found && v.isActive then (k :: (Tuple.first acu), True)
else acu
src/Apps/VirusPanel/Update.elm, line 230 at r2 (raw file):
|> Maybe.map Database.getVirusInstalled |> Maybe.map (Dict.foldr folder ( [], False )) |> Maybe.map Tuple.first
Pls, only run Map once!
src/Apps/VirusPanel/View.elm, line 222 at r2 (raw file):
virusTimestamp hackedServers nip = let -- HORA DO SHOW P****
stop doing this! Soon our code will look like a circus!
src/Apps/VirusPanel/View.elm, line 223 at r2 (raw file):
let -- HORA DO SHOW P**** showTime str =
use pipes
src/Decoders/Account.elm, line 34 at r1 (raw file):
I can't delete the dummies before the bootstrap been totally completed.
delete the dummies, use optional with initial models as fallback, keep a issue to remove it! (actually, create issues for every decoder you see hardcoded/optional)
src/Game/Account/Database/Models.elm, line 115 at r2 (raw file):
getActiveVirus server = server.virusInstalled |> Dict.filter (\k v -> (==) True v.isActive)
(==) True v.isActive
equals v.isActive
You can also \_ -> .isActive
this would generate the nicer JS...
src/Game/Account/Database/Update.elm, line 128 at r2 (raw file):
|> Maybe.map (flip (Dict.insert fileId) viruses) |> Maybe.map (\iv -> { server | virusInstalled = iv }) |> Maybe.map (flip (Dict.insert nip) hackedServers)
Pls, merge maps... use a lot of >>
src/UI/Style.elm, line 330 at r2 (raw file):
, border3 (px 1) solid (hex "ccc") , borderTopColor (hex "07d") , property "animation" "spinner .6s linear infinite"
Try to add an "animation" to Utils.Css ( :thinking: though this name is in my PR only )
Comments from Reviewable
This should be merged if Max uses "optional" in decoders!
Review status: 15 of 24 files reviewed at latest revision, 8 unresolved discussions.
src/Apps/VirusPanel/Update.elm, line 216 at r2 (raw file):
```elm folder k v acu = if not found && v.isActive then (k :: (Tuple.first acu), True) else acu ```
Done.
src/Apps/VirusPanel/Update.elm, line 230 at r2 (raw file):
Pls, only run Map once!
Done.
src/Apps/VirusPanel/View.elm, line 222 at r2 (raw file):
stop doing this! Soon our code will look like a circus!
What are you talking about? It's the same comment as before moved to other function.
src/Apps/VirusPanel/View.elm, line 223 at r2 (raw file):
use pipes
Done.
src/Decoders/Account.elm, line 34 at r1 (raw file):
delete the dummies, use optional with initial models as fallback, keep a issue to remove it! (actually, create issues for every decoder you see hardcoded/optional)
Deleted Bounces.Dummy, Database.Dummy and Finances.Dummy
src/Game/Account/Database/Models.elm, line 115 at r2 (raw file):
```(==) True v.isActive``` equals ```v.isActive``` You can also ```\_ -> .isActive``` this would generate the nicer JS...
Done.
src/Game/Account/Database/Update.elm, line 128 at r2 (raw file):
Pls, merge maps... use a lot of ```>>```
Done.
src/UI/Style.elm, line 330 at r2 (raw file):
Try to add an "animation" to Utils.Css ( :thinking: though this name is in my PR only )
Done.
Comments from Reviewable
Reviewed 18 of 20 files at r2, 3 of 9 files at r3. Review status: 18 of 24 files reviewed at latest revision, 9 unresolved discussions.
src/Apps/DBAdmin/View.elm, line 119 at r2 (raw file):
"USD : " ++ (Maybe.withDefault "?" <| Maybe.map toMoney account.knownBalance) ]
avoid writing code like that when possible:
src/Apps/VirusPanel/Update.elm, line 216 at r2 (raw file):
```elm folder k v acu = if not found && v.isActive then (k :: (Tuple.first acu), True) else acu ```
PS: Pedro's code looks invalid, found isn't defined anywhere.
src/Apps/VirusPanel/Update.elm, line 230 at r2 (raw file):
Pls, only run Map once!
Rewrite using the >>
operator.
And please notice that this code is no problem for haskell, it has rewrite rules to transform
v |> map a |> map b
into
map (a >> b) v
Sadly, we can't rely on that when writing elm.
src/Apps/VirusPanel/View.elm, line 222 at r2 (raw file):
stop doing this! Soon our code will look like a circus!
I think a "no repeated joke" policy is cool, repeated jokes aren't funny.
src/Decoders/Account.elm, line 34 at r1 (raw file):
delete the dummies, use optional with initial models as fallback, keep a issue to remove it! (actually, create issues for every decoder you see hardcoded/optional)
Deleting any unused dummy is a MUST, used dummies can be kept given that they got an issue tracking them.
Also, please take pedro's comment with a grain of salt, do not use optional for fields that we don't know how to decode yet, the decode has high changes of being wrong and we would probably need to rewrite it :/
Comments from Reviewable
src/Decoders/Account.elm, line 34 at r1 (raw file):
Deleting any unused dummy is a **MUST**, used dummies can be kept given that they got an issue tracking them. Also, please take pedro's comment with a grain of salt, do not use optional for fields that we don't know how to decode yet, the decode has high changes of being wrong and we would probably need to rewrite it :/
Just Like database Decoders.
Comments from Reviewable
Reviewed 2 of 20 files at r2, 6 of 9 files at r3. Review status: 19 of 24 files reviewed at latest revision, 4 unresolved discussions.
src/Apps/VirusPanel/Update.elm, line 216 at r2 (raw file):
PS: Pedro's code looks invalid, found isn't defined anywhere.
PS: you noticed that
src/Apps/VirusPanel/View.elm, line 222 at r2 (raw file):
I think a "no repeated joke" policy is cool, repeated jokes aren't funny.
I'm leaving my ok here
src/Apps/VirusPanel/View.elm, line 223 at r2 (raw file):
Done.
I don't understand the need to use pipes here, case seems more readable :T
In order to get any advantage of using pipes here, remove the str
param of showTime
and use >>
instead of |>
(do that or revert to the version that uses case, it's much more readable)
Comments from Reviewable
Reviewed 5 of 5 files at r4. Review status: all files reviewed at latest revision, 4 unresolved discussions.
src/Game/Account/Database/Update.elm, line 120 at r4 (raw file):
viruses = server |> getVirusInstalled
avoid single pipes
Comments from Reviewable
Reviewed 3 of 3 files at r5. Review status: all files reviewed at latest revision, all discussions resolved.
Comments from Reviewable
Don't merge before Database's Bootstrap is on Helix's master branch
This change is