search

LukeWoodward / SplitsBrowser

Orienteering results analysis
GNU General Public License v2.0
13 stars 9 forks source link

Add IOF <TeamResult> and >TeamMemberResult> parser with a xml file re… #66

Closed lingeandrea closed 4 years ago

lingeandrea commented 4 years ago

…sult and orivenezia 2020 data.

Add Italian languange translation (not tested)

LukeWoodward commented 4 years ago

Hi @lingeandrea,

Thanks for submitting this pull request. I'm afraid it's not in a state where I could merge it, for a number of reasons:

However, the single biggest issue with this PR is that I simply don't know what you are trying to achieve with it! You have provided very few details about what your pull request adds: my best guess is that it adds support for relays, by showing one graph line per team instead of one line per runner.

It looks like you've put a fair bit of effort into this PR, so it feels wrong to just close it. At the same time, it seems there's a fair bit of work required to get it into a shape where I would be happy to merge it. I have to say that I was completely unaware that this was coming: there's no issue in the tracker or any other communication with me about this change. Instead, I just got this pull request more-or-less "out of the blue".

You also mention an Italian translation, but I didn't see one in this PR. If you have written an Italian translation file, I'd be very grateful if you could send it to me. The best way to do this would be to create a separate pull request for it: please don't add it to this PR.

lingeandrea commented 4 years ago

Hi @LukeWoodward, I'm really sorrow fo this "out of the blue" PR. First of all I would like to introduce myself explaining after the PR. I'm a newbe in this gibhub, javascript and PR world and if seems I do not follow netiquette rules is because I do not know them but I will learn them. I'm an orienteering racer and a web site maintainer (www.fisofvg.it). 6 monts ago I included Splitbrowser feature on fisofvg hosting your code (v 3.4.4) and gluing it with simple PHP code to retrieve italian orienteering federation data, store them on dB as you can see on fisofvg.

IOF.xsd permits to fill xml file for an event either specifying

or for all classes as shown in the following figure. [image: image.png] Oribos (https://www.bostek.it/), the main SW used in Italy to manage orienteering race, actually can generate event results following IOF.xsd TeamResult rules. This is what happened 4 weeks ago for an italian race ( https://www.fisofvg.it/splitbrowser/race.php?gara=data/FISO2020252.xml) Initially your parse V3.4.4 told me no competitors for all classes. Starting from this I modified your code as you can see on fisofvg and after I shared codes with PR. Thanks for your suggestions and I will follow them to improve code and PR. Andrea Il giorno mar 25 feb 2020 alle ore 20:41 Luke Woodward < notifications@github.com> ha scritto: > Hi @lingeandrea , > > Thanks for submitting this pull request. I'm afraid it's not in a state > where I could merge it, for a number of reasons: > > - Please don't commit versions of the jQuery and D3 dependencies to > the repository. > - Please don't include real data files in the repository, as there may > be privacy concerns regarding the personal data they contain. > - Please don't modify the splits-graph-dev-template.html file: this is > supposed to be a template file. Developers should create a copy of it and > work from that. > - JSHint reports two lint errors with your modifications to > iof-xml-reader.js. Please fix these. > - 8 of the existing tests fail when run. Please ensure you have fixed > the code and/or tests as appropriate. > - You have provided no new unit tests. Please write some. > - There are style issues with the code: your changes include dead > (i.e. commented-out) code, incomplete function headers, incorrect > indentation, and comments and variable names in Italian when the rest of > the codebase uses English. > > However, the single biggest issue with this PR is that I simply don't know > what you are trying to achieve with it! You have provided very few details > about what your pull request adds: my best guess is that it adds support > for relays, by showing one graph line per team instead of one line per > runner. > > It looks like you've put a fair bit of effort into this PR, so it feels > wrong to just close it. At the same time, it seems there's a fair bit of > work required to get it into a shape where I would be happy to merge it. I > have to say that I was completely unaware that this was coming: there's no > issue in the tracker or any other communication with me about this change. > Instead, I just got this pull request more-or-less "out of the blue". > > You also mention an Italian translation, but I didn't see one in this PR. > If you have written an Italian translation file, I'd be very grateful if > you could send it to me. The best way to do this would be to create a > separate pull request for it: please don't add it to this PR. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > , > or unsubscribe > > . >
LukeWoodward commented 4 years ago

Hi @lingeandrea ,

I've had a further chance to look into this pull request, to think about how I would do it if I were to do this myself and to see whether what you have done fits with how I would do it.

I'm afraid I've decided that I won't be merging this pull request.

There are a number of reasons behind this. Firstly, if I was to implement this feature myself, I wouldn't do it as you have done. I wouldn't create a 'virtual competitor' whose times are made up from the competitors in the team. Instead, I would create a separate 'team' object, where each team contains the team name, and has a list of competitors. This is for a number of reasons:

With this project I have found that it doesn't take too much effort to handle the cases where things work, but there's a lot of time spent handling all of the various cases where things aren't right. In particular, there's one error case in your file staffettaorig.xml which you are not handling. In the W -13 class, there is one team with a missing team member. Viewing this class with your modifications to SplitsBrowser causes the application to generate a lot of JavaScript errors in the browser console, some of the graphs don't work and changing the class doesn't work either: you have to reload the page to get away from this.

You have also made changes to competitor.js, course-class.js and course-class-set.js, but I don't understand why you've made them. Furthermore, when I informed you that tests were failing, it seems you didn't try to understand why the tests were failing, but just modified the tests to get them to pass.

I appreciate your interest in the project and your wish to help, and I'm sorry that I won't be merging this pull request, but I hope you can understand why. I may choose to develop this kind of feature myself at some point in the future, but this is dependent on me having the spare time to do it. In the meantime, however, you can continue to run your modified version of SplitsBrowser on www.fisofvg.it. There's nothing I can do to stop you doing that: the ability to do that is a condition of the licence the application is released under.

lingeandrea commented 4 years ago

Hi @LukeWoodward I agree your reasons that leads you to not merge my code. However I will continue to improve my brach of your code following your suggestions. The changes to competitor.js, course-class.js and course-class-set.js,were born because for me it isn't correct to take into account split times and consequently cum times when a competitor doesn't finish his race while you are right when told me that correnspond tests are only patched to pass and not deeply analysed

Il giorno dom 1 mar 2020 alle ore 17:12 Luke Woodward < notifications@github.com> ha scritto:

Hi @lingeandrea https://github.com/lingeandrea ,

I've had a further chance to look into this pull request, to think about how I would do it if I were to do this myself and to see whether what you have done fits with how I would do it.

I'm afraid I've decided that I won't be merging this pull request.

There are a number of reasons behind this. Firstly, if I was to implement this feature myself, I wouldn't do it as you have done. I wouldn't create a 'virtual competitor' whose times are made up from the competitors in the team. Instead, I would create a separate 'team' object, where each team contains the team name, and has a list of competitors. This is for a number of reasons:

  • Your approach of building up the name of the virtual competitor from all team-member names might work for the two-person teams in your staffettaorig.xml file, but for an event such as the Tiomila, with 10-person teams, the names of the virtual competitors would be very long and would take up a lot of space in the competitor list on the left and in the graphs. I would rather use the team names on the graphs and in the competitor list, with the list of team members available in a tooltip.
  • For a relay with a large number of legs, it might be an idea to filter down to just a single leg. There isn't really a way to do this with your approach.

With this project I have found that it doesn't take too much effort to handle the cases where things work, but there's a lot of time spent handling all of the various cases where things aren't right. In particular, there's one error case in your file staffettaorig.xml which you are not handling. In the W -13 class, there is one team with a missing team member. Viewing this class with your modifications to SplitsBrowser causes the application to generate a lot of JavaScript errors in the browser console, some of the graphs don't work and changing the class doesn't work either: you have to reload the page to get away from this.

You have also made changes to competitor.js, course-class.js and course-class-set.js, but I don't understand why you've made them. Furthermore, when I informed you that tests were failing, it seems you didn't try to understand why the tests were failing, but just modified the tests to get them to pass.

I appreciate your interest in the project and your wish to help, and I'm sorry that I won't be merging this pull request, but I hope you can understand why. I may choose to develop this kind of feature myself at some point in the future, but this is dependent on me having the spare time to do it. In the meantime, however, you can continue to run your modified version of SplitsBrowser on www.fisofvg.it. There's nothing I can do to stop you doing that: the ability to do that is a condition of the licence the application is released under.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/LukeWoodward/SplitsBrowser/pull/66?email_source=notifications&email_token=AOUWPQMG73HKQSFDNBIIT43RFKCNTA5CNFSM4K2TK772YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENNDJPA#issuecomment-593114300, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOUWPQL45JO3AOOXD4LEVP3RFKCNTANCNFSM4K2TK77Q .