Closed renatomassaro closed 6 years ago
Ebert has finished reviewing this Pull Request and has found:
You can see more details about this review at https://ebertapp.io/github/HackerExperience/Helix/pulls/383.
Reviewed 10 of 28 files at r1, 35 of 35 files at r2. Review status: all files reviewed at latest revision, 8 unresolved discussions.
events.json, line 200 at r2 (raw file):
}, "DownloadCracker": { "filters": ["File.Downloaded"],
Also filters File.Deleted
(btw File.Deleted
and Step.Restarted
must be added to the JSON file)
lib/software/event/handler/filesystem.ex, line 25 at r2 (raw file):
# Existing entries being removed # TODO FileDeletedEvent
Create issue
lib/story/internal/email.ex, line 67 at r2 (raw file):
do: generic_send(step, reply_id, :player) def rollback_email(step, email_id, meta) do
doc + spec
lib/story/model/step/macros.ex, line 320 at r2 (raw file):
end defmacro setup_once(object, identifier, do: block),
docme
lib/story/model/story/email.ex, line 90 at r2 (raw file):
end def rollback_email(entry, email_id, meta) do
doc + spec
lib/story/model/story/email.ex, line 99 at r2 (raw file):
end defp do_rollback_email(changeset, email) do
defp
should be placed at the end of the file
test/support/story/helper.ex, line 47 at r2 (raw file):
end @doc """
Move these more important functions up
test/support/story/vars.ex, line 3 at r2 (raw file):
defmodule Helix.Test.Story.Vars do @moduledoc """ This helper will inject (in a non-higienic way!)
Oops
Comments from Reviewable
:zero: :cool:
Reviewed 7 of 7 files at r3. Review status: all files reviewed at latest revision, 8 unresolved discussions.
events.json, line 200 at r2 (raw file):
Also filters `File.Deleted` (btw `File.Deleted` and `Step.Restarted` must be added to the JSON file)
Done.
lib/software/event/handler/filesystem.ex, line 25 at r2 (raw file):
Create issue
Done.
lib/story/internal/email.ex, line 67 at r2 (raw file):
doc + spec
Done.
lib/story/model/step/macros.ex, line 320 at r2 (raw file):
docme
Done.
lib/story/model/story/email.ex, line 90 at r2 (raw file):
doc + spec
Done.
lib/story/model/story/email.ex, line 99 at r2 (raw file):
`defp` should be placed at the end of the file
Done.
test/support/story/helper.ex, line 47 at r2 (raw file):
Move these more important functions up
Done.
test/support/story/vars.ex, line 3 at r2 (raw file):
Oops
Done.
Comments from Reviewable
Closes #333. Closes #274.
Now you can delete your cracker and still proceed with the tutorial! :tada:
TODO:
Steppable.setup
(made it idempotent)StepRestartedEvent
Story.Step
meta when data has been regeneratedNotes
UnlikeDue to a tiny detail (checkpoint (email) metadata is defined on the restart method), this cannot be done. A refactor where we make the whole Step interface 100% declarative would solve this, but there's no time for that.Steppable.start
,Steppable.restart
seems generic (it does not change for each step) and as such can be abstracted into a single function, so I think there's no need to implement it on each Step declaration.Incidental
Steppable.fail
toSteppable.restart
. Failure no longer exists :raised_hands:.Steppable.setup/1
into two parts,Steppable.start/1
andSteppable.setup/1
. Now the setup only does the setup, and is reused atSteppable.restart
.Steppable.setup/1
must be idempotent, so existing code was updated.setup_once
macro to quickly enable idempotent object search/creation atSteppable.setup
FileDeletedEvent
. Proper handlers onFilesystemHandlers
are missing.StoryQuery.Setup
for standardized idempotency search duringSteppable.setup
story_emails
This change is