-
Notifications
You must be signed in to change notification settings - Fork 399
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
Added distinct plane type count #992
base: master
Are you sure you want to change the base?
Conversation
Added count of unique plane types, based on IATA.
@@ -74,7 +74,7 @@ | |||
} | |||
|
|||
// unique airlines, unique planes, total distance (mi), average distance (localized), average duration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment presumably wants updating to match...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And because GitHub CR is daft, I can't leave this comment at line 84 below... But presumably this doesn't actually result in anything being output, on the website, it just makes it available to JS, so would also need a change to
Lines 1558 to 1564 in 89643b0
table = "<table style=\"border-spacing: 10px 0px\">"; | |
table += "<tr><th colspan=2>" + gt.gettext("Unique") + "</th></tr>"; | |
table += "<tr><td>" + gt.gettext("Airports") + "</td><td>" + uniques["num_airports"] + "</td></tr>"; | |
table += "<tr><td>" + gt.gettext("Carriers") + "</td><td>" + uniques["num_airlines"] + "</td></tr>"; | |
table += "<tr><td>" + gt.gettext("Countries") + "</td><td>" + uniques["num_countries"] + "</td></tr>"; | |
table += "<tr><td>" + gt.gettext("Vehicles") + "</td><td>" + uniques["num_planes"] + "</td></tr>"; | |
table += "<tr><td> </td></tr>"; |
Obviously doesn't block it being merged, but the comment should probably be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make the necessary changes and submit another pull. Thanks @reedy
This PR seems dead but, nevertheless, I don't think this change is right. Most importantly, the website already shows the plane type count (Analyze / Unique / Vehicles). This information is collected on the line that's being edited: COUNT(DISTINCT plid) AS num_planes The Individual planes are not catalogued by the system but can be tracked by the user. These would be identified by the I'm not aware of IATA assigning any codes to plane types. They do assign airline and airport codes and the database stores these codes in the |
Maybe this change needs a little more design behind it. I will take some time to think about what all would need to be changed overall to make the unique plane identifier available and used in the count. I wish I would have put more comments in my original PR. To @chrisrosset's point, it would be helpful to have a count by registrations as well. |
@reedy, thank you for the link! Today I learned.
@ssegraves Could you please take a look at https://github.com/jpatokal/openflights/blob/master/data/planes.dat and double-check if the middle column (e.g. I think you'd need to left join with |
The issue I see with this is that currently users can specify their own planes. For example, planes.dat only includes CRJs as "Canadair Regional Jet." I have standardized on calling them all "Bombardier CRJ-xxx" in my account, since they have long been branded as that. (Technically, they should become Mitsubishi at this point I guess). So I don't think that my entries are going to match up with the IATA/ICAO codes in planes.dat unless I'm missing something. Incidentally, I have an outstanding MR for planes.dat to add the COMAC ARJ-21 - maybe one of you guys can merge that in? https://github.com/jpatokal/openflights/pull/1025/commits |
No description provided.