Skip to content

Sync allergies with problem-specifications#412

Merged
devkabiir merged 2 commits intomainfrom
sync-allergies
Oct 28, 2022
Merged

Sync allergies with problem-specifications#412
devkabiir merged 2 commits intomainfrom
sync-allergies

Conversation

@kytrinyx
Copy link
Copy Markdown
Member

This puts all the tests in the same group rather than
grouping them per the canonical-data.json structure in
problem-specifications.

In order to help make the logical groupings apparent,
the test names have been updated to be prefixed with the
group description.

To make it as easy as possible to review, I've indented
the tests two spaces extra in the first commit, and then
fixed the indentation in the second commit.

In the third commit I've synced the allergies exercise by running

bin/configlet sync --update --exercise allergies

This brought in updated instructions and an extra test.

@kytrinyx
Copy link
Copy Markdown
Member Author

@devkabiir Would you point me in the right direction here? I've run dart format on the file that is failing, but it seems like CI is checking for something different.

@devkabiir
Copy link
Copy Markdown
Contributor

devkabiir commented Oct 26, 2022

By default dart format has a line length set to 80. We use 120 in the track. So the command you are looking for is dart format -l 120 .. Another way to check and fix auto-fixable things is to run dart run bin/presubmit.dart. That also runs all tests (all exercises) so it might take some time to complete, so I suggest the leaner dart format -l 120 .

I also noticed that we are using an outdated command in the bin/presubmit script. Which is another matter. And we have some exercises with outdated dart style issues, which can be fixed by running dart format -l 120 --fix .

P.S. Another shortcut I forgot is to use the EXERCISE env var to limit the presubmit script to only run checks affecting that exercise. e.g.

EXERCISE=allergies dart run bin/presubmit.dart

Copy link
Copy Markdown
Contributor

@devkabiir devkabiir left a comment

Choose a reason for hiding this comment

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

  • I understand the Sync allergies with problem-specifications part of this PR.

  • I don't understand why tests had to be grouped in a top-level group? Is it because of the missing - in the test case descriptions? as in
    list when: - just eggs vs list when: just eggs

  • If this is exercism wide change, would it mean all exercise test files have to be updated?

@kytrinyx
Copy link
Copy Markdown
Member Author

kytrinyx commented Oct 26, 2022

The change to the structure is based on the response from @Stargator here: #410

I could group them based on the groupings in the canonical-data.json but my understanding was that @Stargator preferred a flat structure.

This is not an Exercism-wide change, I just want to go through the exercises in this track and update them with additional tests etc.

@kytrinyx
Copy link
Copy Markdown
Member Author

Oh my goodness, the tests are passing 🎉 The -l 120 was what I needed. Thank you!

@devkabiir let me know if there's anything I can do differently to make it easier to review for the next exercises that I update.

Comment thread exercises/practice/allergies/.docs/instructions.md Outdated
Comment thread exercises/practice/allergies/.meta/tests.toml
Comment thread exercises/practice/allergies/.meta/tests.toml
Comment thread exercises/practice/allergies/test/allergies_test.dart Outdated
This uses the same structure as in word-count where the groups
are referenced from within main, but defined below it.
Copy link
Copy Markdown
Contributor

@devkabiir devkabiir left a comment

Choose a reason for hiding this comment

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

Let's merge this one now. We can iterate and fine tune the test generator in subsequent PRs. The main objective is to sync the exercise with problem spec. Which is accomplished.

@kytrinyx
Copy link
Copy Markdown
Member Author

I only did the first step, which is to regenerate the existing test suite.
When I went to do the second step and sync with problem-specifications I was reminded about the asterisks in the unordered list in the new markdown, so I went and submitted a PR to fix that in the problem-specifications repository.
exercism/problem-specifications#2138

Once that is merged I'll do the sync and regenerate.

(I like to do it in two steps so that it's clear what changes the sync brings in.)

This brings in updated instructions and an extra test.
@kytrinyx kytrinyx marked this pull request as ready for review October 28, 2022 08:30
@kytrinyx
Copy link
Copy Markdown
Member Author

I've synced and regenerated.

This should be ready to go.

@devkabiir devkabiir dismissed Stargator’s stale review October 28, 2022 13:27

Requested changes addressed

@devkabiir devkabiir merged commit f02fe92 into main Oct 28, 2022
@devkabiir devkabiir deleted the sync-allergies branch October 28, 2022 13:27
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 this pull request may close these issues.

3 participants