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

Add cater waiter concept exercise #818

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

depial
Copy link
Contributor

@depial depial commented Oct 21, 2024

I've translated the python version of cater-waiter. It was quite involved, so please have a close look. I also translated the introduction.md, which I'm not sure if that could be used for in WIP draft of sets concept

I did stray from using proper REPL output, in favor of what gets output if it is wrapped in println. This was because longer collections get unwieldy otherwise.

Another point is that the doc strings for the functions may be a bit Pythonic, since I just did a quick translation.

I'm not sure if we want to merge this or not, since it would publish the exercise to the site.

Edit: Ugh... I'll have a look at why all the checks are failing tomorrow

@colinleach
Copy link
Contributor

colinleach commented Oct 21, 2024

I'll look at this properly tomorrow. One detail I noticed is the missing hints.md, which has been discussed on the forum recently. There is quite a lot of pressure to include both hints (to help mentors as well as students) and a meta/design.md (to prevent feature creep).

To generate the config.sys entry, it is easiest to use configlet create --concept-exercise cater-waiter -a depial, though you still need to fill in some fields. Status is "wip", concepts is whatever, prereqs are pretty much unknown at this stage and will need to be added later.

After a number of past mistakes, I find it helpful to run bin/configlet fmt and bin/configlet lint from the root directory of the repo (assuming you're on something *nix-like, I'm not sure about Windows).

@depial
Copy link
Contributor Author

depial commented Oct 21, 2024

As far as the configlet warnings:

  • I think I understand why the slug is named sets.py in the Python original, and I think making that switch is what the first warning is about. I'll make that switch and see if that helps.
  • I stayed away from translating hints.md since it so heavily references other Python concept exercises. Dereferencing it will leave it quite minimal, but I'll get on that, and we can likely fill it out as the syllabus progresses.
  • I don't particularly care for being listed as an author on such things and have been leaving the author blank on practice exercises, but it looks like it's necessary to fill out for concept exercises.

As for the other failures, it's a bit harder to tell what's going on there (some sort of recursion issue?). There could be a problem with how exemplar.jl includes sets_categories_data.jl, which is in the directory above. I left the solution to that which worked on my machine, but I'm not sure if that is still valid here. I can think of two things to try:

  1. Try changing include(joinpath(dirname(@__DIR__), "sets_categories_data.jl")) in exemplar.jl to include("sets_categories_data.jl") to see if that will work (as it seems to on the Python track).
  2. Put a copy of sets_categories_data.jl in the .meta folder, so it can be accessed directly through include("sets_categories_data.jl")

I'll give the first option a go, because that would be the cleanest, and if it doesn't work, I'll try the second.

Suggested change
@depial
Copy link
Contributor Author

depial commented Oct 21, 2024

I think I've go most of this straightened out. Let me push those changes before making any new ones

@colinleach
Copy link
Contributor

Oops, I was trying to add a suggested change, but accidentally committed it - sorry! In any case, as you say, leaving the author blank is not an option. GH already lists you as the author, so does having this show up on Exercism make things so much worse?

I was working on this locally while you were writing your comment, so I just saw that. Some tentative comments:

  • I think there are too many file imports. If sets_categories_data.jl is imported in cater-waiter.jl, also importing it in runtests.jl and sets_test-data.jl confuses things. Locally, I just got a bunch of redefinition warnings, but maybe the CI is getting more deeply confused?
  • We can work on hints.md and .meta/design.md later, once we've sorted the more urgent problems.
  • I was a bit surprised that you translated introduction.md from Python. This has to be a subset of the concept's about.md, so anything new you add here needs to be copied to about.md.

@colinleach
Copy link
Contributor

On file imports: I meant to point out that Python doesn't do this, it just imports specific items from the files. Also, the language seems more tolerant of overwrites than Julia (perhaps because it has no real constants).

@colinleach
Copy link
Contributor

I'm off to breakfast. I've not yet had my first cup of coffee, and I'm feeling it...

@depial
Copy link
Contributor Author

depial commented Oct 21, 2024

To generate the config.sys entry, it is easiest to use configlet create --concept-exercise cater-waiter -a depial, though you still need to fill in some fields.

This is how I initially created it, but it seems to have broken when I added sets to concepts array.

Also, the language seems more tolerant of overwrites than Julia (perhaps because it has no real constants)

I'm pretty sure this is due to me using const for the constants in sets_categories_data.jl, so I've tried dropping them.

I was a bit surprised that you translated introduction.md from Python.

I got a bit confused since there appears to be a nearly empty introduction.md in the WIP draft of sets concept changed files, so I went ahead and translated this.

@colinleach
Copy link
Contributor

This is how I initially created it, but it seems to have broken when I added sets to concepts array.

That was my mistake yesterday, when I didn't read the error carefully enough. It's not your config.sys, the problem is that the linter can't find a sets concept. Some options:

  • Leave the "concepts" field empty for now (though the linter may also complain about that).
  • Finish and merge the sets concept.
  • Ignore the linter, and merge your exercise.

I'm pretty sure this is due to me using const for the constants in sets_categories_data.jl, so I've tried dropping them.

Well, your choice, but I think all the includes might remain a problem. Let's find out empirically.

here appears to be a nearly empty introduction.md

A miscommunication, though we had some earlier discussion about leaving the introduction till last, then chopping down the about to just what is needed for the exercise. That's Bethany's recommendation, and she tends to be right!

@colinleach colinleach closed this Oct 21, 2024
@colinleach colinleach reopened this Oct 21, 2024
@colinleach
Copy link
Contributor

Clearly, not enough coffee yet - I hit the wrong button...

@depial
Copy link
Contributor Author

depial commented Oct 21, 2024

Configlet passes now. The other error does seem to be associated with trying to include sets_categories_data.jl in exemplar.jl. The only other exercise which uses a similar setup is alphametics, since it has an extra file for permutations.jl. However, the example.jl simply wrote the contents of that file directly in the the example, so it doesn't need to be included. I did this and it seems to have cleared up one problem, but now there are others.

A miscommunication, though we had some earlier discussion about leaving the introduction till last, then chopping down the about to just what is needed for the exercise.

Your right, I forgot about this.

Lunchtime for me, so I'll be back to look at this later

@depial
Copy link
Contributor Author

depial commented Oct 21, 2024

It looks like I've been on autopilot for the past day and haven't been thinking... I realized that I tried to use an outside file with helper functions for test generation with my first attempt at that Perceptron exercise, and it failed in a similar manner. The solution was to include everything in the runtests.jl file and wrap that in a function, so I could put the potentially spoiler code at the end.

Since the files sets_categories_data.jl and sets_test_data.jl are so large, I don't think that adding them directly into runtests.jl is a great option. For that reason, we might have to abandon the use of this exercise, and go with something less proprietary, or maybe I can find a way to restructure this one to have less detail.

Any thoughts?

@colinleach
Copy link
Contributor

Let me play with it later. I'm pretty confident we can fix this, but I need to prove it.

I'm close to getting array-operations completed and it would be good to get that sorted before I lose my train of thought (the risk increases with ageing, unfortunately!).

@depial
Copy link
Contributor Author

depial commented Oct 21, 2024

I've got it tentatively "working", presuming that the include("sets_categories_data.jl") that is in the slug will provide access to that file for runtests.jl when a student submits their code. Effectively, I put the data from sets_test_data.jl at the bottom of sets_categories_data.jl and put both of those explicitly in the exemplar.jl.

If you want to play around a bit more to see if you can clean things up (as I find this to be a messy solution), feel free (at a minimum sets_test_data.jl is now superfluous). In case you want to continue tinkering, I'll give you the heads-up that the issue that I couldn't overcome before (when working with other Colin) is the use of other files in runtests.jl (which would be the use of sets_test_data.jl in this case). It appears as though the inclusion of anything other than the slug is ignored.

I'm close to getting array-operations completed and it would be good to get that sorted before I lose my train of thought (the risk increases with ageing, unfortunately!).

I've also got some other things on my plate for the next few days of this week, so I'll be more sporadic in my responses here, but I'll keep thinking to see if there's a better solution.

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.

2 participants