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

#48in24: Meetup 2024-08-20 and PHP is a featured language #788

Merged
merged 8 commits into from
Aug 19, 2024

Conversation

sarad1p1ty
Copy link
Contributor

@sarad1p1ty sarad1p1ty commented Aug 17, 2024

As per #667:

Meetup will be the only exercise with PHP as featured language during #48in24! Let's prepare it well:

  • Run 'bin/configlet sync -u -e meetup --yes --docs --filepaths --metadata --tests include' (updates the Markdown files and maybe 'tests.toml')
    • docs: instructions unsynced: meetup
    • metadata: unsynced: meetup
  • 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

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.

In addition to the other sync-ing tasks, consider doing these:

Copy link

This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested.

If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos.

[no important files changed]

For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping @exercism/maintainers-admin in a comment. Thank you!

@mk-mxp
Copy link
Contributor

mk-mxp commented Aug 18, 2024

Looks like there is a problem with an exercise you did not change... Could you change this line in composer.json: [...]

That didn't work.

@mk-mxp mk-mxp added x:action/sync Sync content with its latest version x:knowledge/elementary Little Exercism knowledge required x:module/practice-exercise Work on Practice Exercises x:type/content Work on content (e.g. exercises, concepts) x:size/small Small amount of work x:rep/small Small amount of reputation labels Aug 18, 2024
@mk-mxp
Copy link
Contributor

mk-mxp commented Aug 18, 2024

@sarad1p1ty I solved the problem now. Please rebase this PR on the latest main, CI should pass then.

@sarad1p1ty
Copy link
Contributor Author

Thank you for that. Hopefully all good now.

Apart from adding DocBlocks, the basic tasks were mostly already done :)

I'd like to neated up the test code, but wanted to run the format by you first, eg:

/**
     * uuid: d7f8eadd-d4fc-46ee-8a20-e97bd3fd01c8
     * @testdox when teenth Monday is the 13th, the first day of the teenth week
     */
    public function testMonteenthOfMay2013(): void
    {
        $expected = new DateTimeImmutable("2013/5/13");
        $actual = meetup_day(2013, 5, "teenth", "Monday");
        $message = "Your function did not return the correct date.";
        $this->assertEquals($expected, $actual, $message);
    }

Something like that?

@sarad1p1ty
Copy link
Contributor Author

I'm also not quite sure how to action #617 ?

@mk-mxp
Copy link
Contributor

mk-mxp commented Aug 19, 2024

@sarad1p1ty Thank you for updating the branch. I think you cannot quickly solve #617 or one of the other possible action points. Those were meant for a long term preparation - which didn't happen.

        $message = "Your function did not return the correct date.";

I wouldn't do that. That is the obvious reason for failing tests. I think I wrote that action suggestion after facing some complicated, hard to understand tests in another exercise. It doesn't make sense now, after looking at all the actual tests.

The other suggested changes look good, with 2 additional spacing lines to make the Arrange, Act, Assert pattern stand out:

    {
        $expected = new DateTimeImmutable("2013/5/13");

        $actual = meetup_day(2013, 5, "teenth", "Monday");

        $this->assertEquals($expected, $actual);
    }

Copy link
Contributor

@mk-mxp mk-mxp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this in the evening. We may add the structural changes later, too.

Thanks a lot for your contribution!

@sarad1p1ty
Copy link
Contributor Author

Thank you for your help.
Sorry, I messed up the git rebase. I'm going to quit now while I still can.

Copy link
Contributor

@mk-mxp mk-mxp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what kind of git trouble you are facing...this PR looks great. Thank you very much!

@mk-mxp mk-mxp merged commit ee323bc into exercism:main Aug 19, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:action/sync Sync content with its latest version x:knowledge/elementary Little Exercism knowledge required x:module/practice-exercise Work on Practice Exercises x:rep/small Small amount of reputation x:size/small Small amount of work x:type/content Work on content (e.g. exercises, concepts)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants