Add support for matches

[Olen]

Should probably refactor event to a class first, so match can be a subclass of event

[Olen]

New parameters for matches:

 'matchEvent': True,
 'matchInfo': {
               'opponentName': 'Hasle-Løren Hvit',
               'opponentScore': 6,
               'scoresFinal': True,
               'scoresPublic': True,
               'scoresSet': True,
               'scoresSetEver': True,
               'teamName': 'Grüner lilla',
               'teamScore': 11,
               'type': 'AWAY'
},

type can be HOME and AWAY

[elliot-100]

Can't help with this one -- at least not with real-world examples -- as Matches aren't relevant/available to my clubs (documentation on which sports are supported).

[Olen]

They are just regular events, but with a few more fields (noted in the first comment)

[Bart274]

This is something that would be really useful. Any progress on this?

[elliot-100]

Olen raised a PR against my separate Spond-classes package: elliot-100/Spond-classes#148

I should be able to add it in the next few weeks.

[Bart274]

Nice. That would really help us a lot 😊

[elliot-100]

@Bart274 can you have a look at the PR elliot-100/Spond-classes#148 please? I don't think I can tag you from there.

[Bart274]

@elliot-100 it looks good. But I don’t see why that PR is blocking any development for this issue here? I use the spond package without the spond-classes at this moment.

[elliot-100]

@Bart274 given:

They are just regular events, but with a few more fields

I'm not clear what you want spond to do? Could you give an example?

[Bart274]

@elliot-100 well, the matchinfo fields are missing in the event template:

Spond/spond/_event_template.py

Line 5 in fe6db62

_EVENT_TEMPLATE: JSONDict = {

Since these fields are missing, even when you retrieve an existing event from Spond that contains the data, whenever you call the method to update existing events, it will remove the data from the event in the function as it’s keys are missing in the template.

Spond/spond/spond.py

Line 354 in fe6db62

for key in base_event:

So we can’t update the matchinfo of events through this API.

[elliot-100]

OK, can you raise a separate bug issue instead, please, as this doesn't really relate to adding a class interface for events.

(although I can see that the list of additional params might give that impression)

[Bart274]

@elliot-100 but isn’t this the point of this ticket? The issue is to add support for matches… 😄
We can retrieve the matchinfo from existing events just fine, but we can’t update them. What else is this ticket for?

[elliot-100]

The package does support matches in general, like any other event.

@Olen, who is the author of the package, created this issue and has decided that at this time, what they want is best implemented as an addition to my existing class interface. And so has raised a linked closing PR there. That is their perogative.

It was not clear from your initial supporting comment in the conversation that you were expecting something else. So I (the other main contributor to this package) have just spent some development time on this during holidays, partly in order to help you.

You could now help us and other users and contributors by raising a separate issue, so we, and you, can track it more easily. For example you might wish to raise a PR yourself against that in order to contribute further.

[Bart274]

@elliot-100 matches are supported to retrieve information. However, since the matchevent & matchinfo keys are missing in the event template, you can't use the update_event method to update that information.
I have added that information in the event template in this PR: #169

[Olen]

My main point was that I did not want to do any major changes here before the classes PR was merged, as I really think that is the way forward.

I have not followed the development of that branch tightly, so I am not sure how close to ready that branch is.
But if it feals mature enough, I am more than happy to merge it here and create a breaking release.

[Bart274]

@Olen & @elliot-100 I don't see why the spond-classes PR should be merged first?
I thought the spond-classes package is an additional tool to handle the data coming from this package to have objects instead of dictionaries?
The change in the spond-classes package should support loading the event data from Spond as a Match object that includes the matchinfo.
However, the change is that package doesn't solve the issue we have with the update_event function in this package. This function uses the event template from this package to update existing events, but this template doesn't contain the matchinfo, so we will never be able to update the matchinfo for existing events, no matter the change in the spond-classes package?
That's why I added #169 to make sure we can update these values for existing events.
This is a change that needs to happen in the base functionality of this package.

[Olen]

The Classes Branch is a complete rewrite of the project to make it more OO.
This project started out as a very simple tool so I could create an ical-calendar I could subscribe to instead of giving the Spond-app access to my calendar on the phone to add events there.
It has since grown quite a lot, and could really need some improvements. I had thought for a while about doing that - including using classes instead of simple functions to ensure consistency and better error handling. But @elliot-100 beat me to it, and has made huge improvements, both in that branch, and to the project over all.

I do not object to #169 per se, but I think it should be better tested and also that proper tests should be written before it can be merged.

As I tried to explain - MY hope is that the "Classes" branch will be ready soon, so that we can benefit from all the improvements, and that work should be focused on getting that merged. And even if it will contain breaking changes, efforts should be made to minimize those, and that also means that it needs a somewhat "stable" version of the main branch, so it does not have to chase lots of changes here.

[elliot-100]

I thought the spond-classes package is an additional tool to handle the data coming from this package to have objects instead of dictionaries?

This is correct, as per the README code and description. It's not a branch, or a general rewrite, and doesn't provide any way to get data out of the Spond API (it doesn't e.g. wrap/adapt this package's functionality), let alone into it.

Yes it could potentially be merged or adapted into this project as some point, and of course anyone is welcome to have a go, but to be honest there doesn't appear to be any demand for this - I'm the only known user, and it already covers what I need/find useful; I occasionally add support for individual fields as the need arises, or as a learning exercise.

Where I have seen issues or ways to improve this project's codebase, I have contributed here, and will try to continue to do so. I think most of those contributions have addressed/are addressing issues that get in the way of adding a class interface, whether it's mine or another.

So getting back to this specific issue - I feel @Bart274 is right in that this project's #169 should be considered on its own merits, irrespective of elliot-100/Spond-classes#148. I don't deal with matches as my clubs don't have them, so I can't really provide any real-world assurance.

[Olen]

Oh, then I have misunderstood, and I am sorry about that.

I will probably create a new "classes"-branch here, and try to merge the two, as I really prefer objects and classes when the project grows like this.

[elliot-100]

Well, I think I misunderstood/misspoke earlier in this thread.

I think I have notes on some additional proposals for this project which may help add the class interface - I will review and see if anything looks like it can be raised as an enhancement and/or PR.

And of course I am happy to add to Spond-classes too, or assist on integration, if it helps you or someone else.

[Olen]

I just quickly created a branch and added the files from your repo.
https://github.com/Olen/Spond/tree/oo-rewrite

Then I can start modifying the functions to return proper objects and that will hopefully make it easier to maintain comaptibility later.

[Olen]

(But I still think some tests should be added to #169 and I would like at least some verifications from a few users that it does not break events that are not matches)

[elliot-100]

I just quickly created a branch and added the files from your repo.
https://github.com/Olen/Spond/tree/oo-rewrite

Then I can start modifying the functions to return proper objects and that will hopefully make it easier to maintain comaptibility later.

To be honest I think there's other work that needs doing first.

I think the project has significant technical debt, and it would make sense to spend some time e.g. considering how some of the issues I've raised with the existing gets/puts might be resolved.

Some functions just don't work properly, because they've just had extra features added over time without later retrospection.

I also think the project would really be helped by setting some code standards for v2.x before writing any new code - including enforced linting, requiring tests for new features, enforced type checking, better (automated) documentation.

I don't mean any of this as a criticism. It's the first real code project I have ever worked on, and I have certainly made a lot of mistakes along the way.

I can certainly propose some of this for 2.x. Perhaps take a look at where I've got to with Spond-classes linting/CI/docs and let me know your thoughts.