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

[New Practice Exercise]: Global Meetup Basic draft #3446

Closed
wants to merge 12 commits into from

Conversation

safwansamsudeen
Copy link
Contributor

@safwansamsudeen safwansamsudeen commented Jun 19, 2023

For background, check exercism/problem-specifications#1923 and linked forum posts.

This is a very basic draft. As it's my first exercise contribution, I thought I'd move slowly.

First thing, the story is a little corny, but I thought it follows the "human" story policy. Feel free to change. Next, this exercise can currently be solved without datetime, as I've shown in the example answer. How can we change the exercise to force datetime usage, or should we not do that?

I also am not sure about the amount of tests to add: obviously, this isn't enough, but how many is?

I realize I should change the track config.json, so here's what should be there: only I'm unclear about the last three keys. I feel it'd make more sense to finalize them after the exercise is more or less complete.

{
    "uuid": "2d9e74e6-d8ac-442f-9d08-165a896e179f",
    "slug": "global-meeting",
    "name": "global-meeting",
    // I'm unclear about the following lines
    "practices": ["if-statements", "numbers", "operator-precedence"],
    "prerequisites": ["if-statements", "numbers"],
    "difficulty": 1
}

I haven't yet set up the linter and formatter, so apologize for any errors in that end. Will be fixed over the coming days.

If you'd like to discuss about the improvement of the exercise from a non code perspective, please do so on the forum.

Thanks!

@github-actions

This comment was marked as resolved.

@github-actions github-actions bot closed this Jun 19, 2023
@BethanyG BethanyG reopened this Jun 19, 2023
@BethanyG BethanyG changed the title Basic draft [New Practice Exercise]: Global Meetup Basic draft Jun 19, 2023
@BethanyG BethanyG closed this Jun 19, 2023
@BethanyG
Copy link
Member

BethanyG commented Jun 19, 2023

@safwansamsudeen - so I am going to hold off doing any code review until the discussion you started in the forum is settled. I'll take another look at the plylint issue which I promptly forgot to do from the last time.

From first glance, this is actually not what I had in mind, so I apologize for being vaguer than I should have been.

For Python, our specific need is to have a series of practice exercises that will help students navigate the details of the datetime module and related topics around calendars, timezones, time formatting, and time duration. We probably want this to be a level 2-4.

Clearly, not all of that should go into this one exercise (I am personally visualizing an eventual cluster of exercises), but we do need to craft it for Python so that it encourages students who would be theoretically coming from a datetime concept exercise to practice different techniques.

To that end, "prerequisite" and "practices" keys in the config.json would both be datetime (I'd have to think about what additional prerequisites might be). When a concept is set in a "practices" key on a practice exercise, that practice exercise then shows up on the syllabus tree under the concept. So if you take a look at the practice exercise square root in config.json you will see that it has numbers in its "practices" key:

Practices Example image

And that translates to the following in the exercise tree:

image

The "prerequisites" key controls which concept and practice exercises a student must complete before the exercise becomes unlocked and available if the student is using learning mode. Right now, I have datetime following classes, so that is fairly far down the proposed syllabus tree:

Tree snapshot image

 

...

 

So classes would probably be a prerequisite, as well as datetime. But there is also a possibility of adding in other practice exercises, so that the student needs to complete for example gigasecond (which can be solved using a timedelta object) to then unlock this one.


The story appears to lend itself right now to a strong notion of timezones, so we may want to double down on that and look at scenarios where an employee is in an inconvenient local, or on or near and international dateline to set that up. Some of that can be done by crafting input or test data. Or we can more firmly point students to that in the intro or directions.

Another thought might be to focus on date formatting, and shift the focus to having a student convert something like a UTC timestamp to the various employee locales/calendars and correctly format it for display.

For amount of test cases, I'd look at exercises like Meetup, Gigasecond, and Clock for inspiration. The goal would be to cover a reasonable amount of "happy path" scenarios, with some edge cases that challenge a student to think through "gotchas". But I also think we need to figure out what specific features of dates and times we will be covering before we go too far down the path of discussing tests.

@BethanyG BethanyG reopened this Jun 19, 2023
@safwansamsudeen
Copy link
Contributor Author

The story appears to lend itself right now to a strong notion of timezones, so we may want to double down on that and look at scenarios where an employee is in an inconvenient local, or on or near and international dateline to set that up. Some of that can be done by crafting input or test data. Or we can more firmly point students to that in the intro or directions.

I don't fully understand - "where an employee is in an inconvenient local, or on or near and international dateline to set that up"?

Another thought might be to focus on date formatting, and shift the focus to having a student convert something like a UTC timestamp to the various employee locales/calendars and correctly format it for display.

That sounds good! But by itself, I doubt it'll come to level 2 - we need to combine it with something else.

OK. Here's a suggestions for this story: you want to arrange the meeting (that's already done) on a specific day UTC. After that, you want a dictionary with each employee and their local time formatted, with the local date. That makes it significantly harder to implement this without datetime.datetime and datetime.timedelta.

Now that I realize you want multiple exercise (we don't have a concept exercise for this, I believe - are you working on that?), another idea would be to allow students to utilize .strptime and .strftime, which would be interesting and useful (and would teach date formatting).

As such, we can't implement a better time zone version (as tzinfo is merely an ABC) without isolating previous Python versions. We do have zoneinfo from 3.9, which we might use to make this better - pass in timezone names instead of the offset. I like it, as it's a useful and relatively new Python module. However, I think we must support 3.7+ - or is that not a requirement?

@BethanyG
Copy link
Member

I don't fully understand - "where an employee is in an inconvenient local, or on or near and international dateline to set that up"?

So - as one example, if you or I were to set up an "office meeting", one of us would have some pretty major inconvenience. My current local time (as I write) is around 2:30am (I'm in PDT), and I suspect your local time is around 3:00pm (IST) if I have your location correct. 😄 And just think if we needed to add in our (imaginary) colleagues from Auckland!

Now those extremes are me being rather radical and unfair, but that's what that comment was in reference to. And of course adding in Auckland just doesn't work...because they are close to the international dateline. It's already Tuesday for them by the time most people are waking up in my part of the planet.

Here is a snapshot

A more real-world scenario (one I often navigate) is setting up a meeting that includes several colleagues from London, a few from the New York, a few from Chicago, one from San Francisco, and one from Berlin. Someone has to get up early or stay late in the office -- but who? So timezone shenanigans. 😄

So yeah - you nailed it:

OK. Here's a suggestions for this story: you want to arrange the meeting (that's already done) on a specific day UTC. After that, you want a dictionary with each employee and their local time formatted, with the local date. That makes it significantly harder to implement this without datetime.datetime and datetime.timedelta.

-- I like that. I think we can riff/work on that.


As such, we can't implement a better time zone version (as tzinfo is merely an ABC) without isolating previous Python versions. We do have zoneinfo from 3.9, which we might use to make this better - pass in timezone names instead of the offset. I like it, as it's a useful and relatively new Python module. However, I think we must support 3.7+ - or is that not a requirement?

So "officially" the PSF is ending support for 3.7 this month. But the only thing that means is that they are no longer making & releasing bug fixes in the language for that version. For vendors and library maintainers, it means that they can check if the version is 3.7, and require that a user upgrade before installing their code.

But it doesn't mean that Python 3.7 no longer "works" - there is a backward compatible guarantee - code written in 3.7 will run under 3.8-3.12+. But code written in 3.12 might not run under 3.7.

What that means for the Python track is that I have to change the docs to let students know that if they are using 3.7, they aren't going have the best experience, and can't expect mentors or maintainers to help troubleshoot their local setups. Of course, since our tooling is at 3.11, they can easily use the online editor and tests.

So I don't really get the support question here. If a student chooses to try and locally solve the problem using Python 3.7, and runs into ditch because they don't have access to zoneinfo -- well, we can warn them. But the exercise story and setup shouldn't force someone (beyond using datetime itself) to have a specific strategy or version of Python.

does that make sense?

@BethanyG
Copy link
Member

I will also dig through the Python 3.7 docs to see exactly how bad the zone stuff might be. I have on my list to update references to 3.7. I just haven't gotten to them yet.

@safwansamsudeen
Copy link
Contributor Author

safwansamsudeen commented Jun 19, 2023

So - as one example, if you or I were to set up an "office meeting", one of us would have some pretty major inconvenience. My current local time (as I write) is around 2:30am (I'm in PDT), and I suspect your local time is around 3:00pm (IST) if I have your location correct.

I was considering a call to discuss this, but forget it... :)

-- I like that. I think we can riff/work on that.

Great. So I need to update the exercise to emphasize the day aspect of it.

So I don't really get the support question here. If a student chooses to try and locally solve the problem using Python 3.7, and runs into ditch because they don't have access to zoneinfo -- well, we can warn them. But the exercise story and setup shouldn't force someone (beyond using datetime itself) to have a specific strategy or version of Python.

What I meant that was we either have to pass in the timezones as offset as I've done here, in which case users could still solve it without datetime - or we'd have to pass in timezone names Hydrabad/Asia or whatever - in which case we'd need to use zoneinfo - datetime doesn't support named timezones.

But I think that if we add the day complication as agreed now, we can drop this idea. I mean, sure, users can solve it without datetime, but exercises can always be bypassed - and it's much easier solving it with datetime now.

@BethanyG
Copy link
Member

I was considering a call to discuss this, but forget it... :)

I'm up. I was just about to start working with Meatball on stuff we were doing with dictionaries. If you'd like to join, I think we can manage that. Are you on Discord?

@safwansamsudeen
Copy link
Contributor Author

@BethanyG sure!

@BethanyG
Copy link
Member

We're chatting in Discord now. I sent you a friend request. :-) Just ping when you are ready to join.

@safwansamsudeen
Copy link
Contributor Author

So restructured the exercise as we think - I like this iteration a lot better.

It was rather tough to solve for me 😄. I've put in values in the track config.json as I see fit, lemme know if there should be changes.

What do you think about this version? If it's alright, I'll go ahead and add tests (and the code for errors, which I'm yet to implement).

@BethanyG
Copy link
Member

@safwansamsudeen - I got swamped today, and have yet to review this. However, I will sit down and look this evening (Jun 21, 2023) after 6pm PDT. Apologies that I can't get to it sooner! Thanks for all your hard work on this. 😄 ✨

@BethanyG BethanyG self-requested a review June 21, 2023 23:40
@safwansamsudeen
Copy link
Contributor Author

@BethanyG no problem, I get it! Do take your time.

I notice the explicit datetime...

@safwansamsudeen
Copy link
Contributor Author

@BethanyG just reminding you in case you forgot 🙂.

@BethanyG
Copy link
Member

@safwansamsudeen - thank you for the reminder! I have been both distracted and swamped, but will review this before my weekend expires. 😄

Copy link
Member

@BethanyG BethanyG left a comment

Choose a reason for hiding this comment

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

Hi @safwansamsudeen - Thank you (again!) for all of your hard work on this - it is much appreciated!

Apologies for the delay in getting this reviewed. I am still swamped, so am doing this in stages. I really like the story and the instructions! I did have some suggestions for clarifying and shortening them a bit. I think some of the story might be too detailed for students.

That being said, if you don't like some of my suggestions, you can opt to not incorporate them.

I am going to need to do a second pass on the code, tests, and instruction appends. For example, if we want error handling scenarios, the appends tend to look like this, and we may want to also consider an append about datetime like the one in gigasecond - or maybe one that is more detailed in directing students to strptime, timedelta and timezone.

That being said, I think the append around expected output formats might be a bit too detailed. We tend to have students read the tests for that sort of information.

I am also starting to think that we might want to pass the meeting date as a UTC timestamp and the employee times as a list of timezone aware datetime objects - but I need to think about it a bit more.

config.json Outdated Show resolved Hide resolved
config.json Outdated Show resolved Hide resolved
config.json Outdated Show resolved Hide resolved
exercises/practice/global-meeting/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/practice/global-meeting/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/practice/global-meeting/global_meeting_test.py Outdated Show resolved Hide resolved
exercises/practice/global-meeting/.docs/introduction.md Outdated Show resolved Hide resolved
exercises/practice/global-meeting/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/practice/global-meeting/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/practice/global-meeting/.docs/instructions.md Outdated Show resolved Hide resolved
@safwansamsudeen
Copy link
Contributor Author

safwansamsudeen commented Jun 27, 2023

Thanks for the review!

I haven't yet installed the linter/formatter - did you get to that (if not, I'll open an issue as you instructed and I forgot)?

Removing the practices key causes the lint check to fail, so I set it to an empty list.

That being said, I think the append around expected output formats might be a bit too detailed. We tend to have students read the tests for that sort of information.

The number of times I've argued with Isaac over it 😆. I feel we should be as explicit as possible, but feel free to modify it as you want - I'm not too strong about it.

I am also starting to think that we might want to pass the meeting date as a UTC timestamp and the employee times as a list of timezone aware datetime objects - but I need to think about it a bit more.

That's fine, only it doesn't teach students date formatting. I think date formatting as an exercise for itself would be annoyingly basic, so it's better to include it with something else - what do you say?

I'm assuming that the story is fine, so I'll start work on more test cases and improving the test. Let me know about the changes you'd like!

P.S. - why are the checks failing?

@BethanyG
Copy link
Member

That's fine, only it doesn't teach students date formatting. I think date formatting as an exercise for itself would be annoyingly basic, so it's better to include it with something else - what do you say?

So we can pass in UTC timestamp and datetime objects, but ask students to pass out a dict/list/tuple of formatted strings, which would still have them practicing string formatting.

But I need to think on it some more. Maybe pause working too much on the tests right now, in case things get switched up?

P.S. - why are the checks failing?

The CI "sees" that you have an example solution and a test file. It attempted to run your example solution against your test file, but had an issue detecting/collecting the tests to run - so it failed. It then went on to fail for Python 3.7 and Python 3.9, and then gave up on Python 3.8 due to the other failures. I will take a look.

@safwansamsudeen
Copy link
Contributor Author

@BethanyG what's your decision on this?

BTW, you removed if __name__ == '__main__' in the code review, which left unittest.main. That causes the tests to fail. I've added it back - or if you want it out, we can remove both lines. Other exercises do one of these two. You can also say, it seems, unittest.main(exit=False)` to solve the error, but I doubt that's a solution.

RN, I'll start working on the test cases - even if things change, we merely have to change the output format. The tough bit is finding appropriate cases ;).

@BethanyG
Copy link
Member

@safwansamsudeen

BTW, you removed if name == 'main' in the code review, which left unittest.main. That causes the tests to fail. I've added it back - or if you want it out, we can remove both lines.

I meant to remove both lines. We are in the process of removing that from all test files, but are blocked right now waiting on some upstream adjustments.

RN, I'll start working on the test cases - even if things change, we merely have to change the output format. The tough bit is finding appropriate cases

I'd advise you hold off, since we haven't (yet - my apologies for being really busy) nailed down the details of the exercise.
It's not about format, and more about content. Thanks for your understanding - I know this is taking time, but I am overwhelmed at the moment and can't get to it right now.

@safwansamsudeen
Copy link
Contributor Author

@BethanyG sure!

@safwansamsudeen
Copy link
Contributor Author

Hi Bethany,

Can I restart work on the tests?

@BethanyG BethanyG added the on hold ✋🏽 Action should stop on this issue or PR for now. label Aug 5, 2023
@BethanyG
Copy link
Member

BethanyG commented Aug 5, 2023

@safwansamsudeen - I think we need to hold off on this for the time being. I need to focus my efforts on getting the approaches that have been sitting in the queue done, as well as some concept exercises that have been waiting a while. I don't think I can pick this up before the end of the month. Apologies.

@safwansamsudeen safwansamsudeen closed this by deleting the head repository Aug 19, 2023
@BethanyG
Copy link
Member

BethanyG commented Aug 30, 2023

@safwansamsudeen - Let's keep this closed for now. I still have a bunch of stuff to do, and now have changes and bug fixes to make for the track tooling, so this is going to have to wait a while longer. Thanks.

@BethanyG BethanyG closed this Aug 30, 2023
@safwansamsudeen
Copy link
Contributor Author

@BethanyG any chance of restarting soon?

@BethanyG
Copy link
Member

@safwansamsudeen - I've decided to go in a different direction for datetime, and am focusing on a cluster of concept exercises for now, so I don't think this will be restarting soon. Apologies.

@safwansamsudeen
Copy link
Contributor Author

@BethanyG ah, okay. I'd like to participate, if you have something free?

@BethanyG
Copy link
Member

@safwansamsudeen I'm sorry - but not at this time. Please check back in the new year, I am pretty swamped with tooling and the work we're trying to complete before the end of December.

@safwansamsudeen
Copy link
Contributor Author

@BethanyG any area to help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold ✋🏽 Action should stop on this issue or PR for now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants