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

Sync clock #845

Open
4 tasks done
Tracked by #628
mk-mxp opened this issue Nov 7, 2024 · 4 comments · May be fixed by #863
Open
4 tasks done
Tracked by #628

Sync clock #845

mk-mxp opened this issue Nov 7, 2024 · 4 comments · May be fixed by #863
Assignees

Comments

@mk-mxp
Copy link
Contributor

mk-mxp commented Nov 7, 2024

  • Run bin/configlet sync -u -e clock --yes --docs --filepaths --metadata --tests include (updates the Markdown files and maybe tests.toml)
    • metadata: unsynced: clock
  • Drop strict types comments from test file and example code (these are useless)
  • Add and sync test meta data to tests (uuid / @testdox in DocBlocks)
  • Decide on adding / adjusting / ordering test cases to match current problem specs
    • metadata: unsynced: clock

Do not redesign the student's interface or add test cases that would invalidate existing community solutions. These are extra tasks, which should be discussed in advance.

@tomasnorre tomasnorre self-assigned this Nov 15, 2024
@tomasnorre tomasnorre linked a pull request Nov 15, 2024 that will close this issue
@mk-mxp
Copy link
Contributor Author

mk-mxp commented Dec 8, 2024

To pull the discussion out of the PR:

The implicit(*) object to string conversion $this->assertEquals('08:00', $clock) is a magic trick in PHP, but we currently have no concept (concept exercise) for students to learn about this. So for now we should do at least one of these:

  • Explain the magic in the stub (a link to the PHP docs Stringable interface may be enough?)
  • Explain the magic in instructions.append.md (like above)
  • Avoid the magic and explicitly call the method as done before $this->assertEquals('08:00', $clock->__toString())
  • Invoke the magic explicitly by converting the object to a string $this->assertEquals('08:00', (string)$clock)

I'd prefer having a DocBlock above __toString() in the students stub pointing at Stringable interface in PHP docs and an explicit conversion (string)$clock in the tests. This allows us to use the magic in the tests and helps students find all required information about what __toString() is.

Opinions? @tomasnorre @homersimpsons

(*) Implicit here means: PHPUnit does the conversion instead of us, so it is invisible for students.

@tomasnorre
Copy link
Contributor

I'd prefer having a DocBlock above __toString() in the students stub pointing at Stringable interface in PHP docs and an explicit conversion (string)$clock in the tests. This allows us to use the magic in the tests and helps students find all required information about what __toString() is.

I'm perfectly fine with this. I should not forget we are in a learning scenario.

@homersimpsons
Copy link
Contributor

I agree that we should not let the PHP's implicit cast to string. If we want to keep it I prefer to go with an explicit cast (string). I think that the explanation in the stub has more chances to be read so I would vote to have at least this one.

I'm fine with going more in depth (or repeating the source comment) in the instructions.append.md.

@mk-mxp
Copy link
Contributor Author

mk-mxp commented Dec 10, 2024

Thanks everyone for opinions. So we go with (string)$clock and a comment in the students stub. I'd leave in-depth descriptions for the time adding a concept for magic methods.

@tomasnorre Would you be so kind to update the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants