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 supporting document for autos #1677

Closed
danbri opened this issue Jun 27, 2017 · 31 comments
Closed

Add supporting document for autos #1677

danbri opened this issue Jun 27, 2017 · 31 comments
Assignees

Comments

@danbri
Copy link
Contributor

@danbri danbri commented Jun 27, 2017

Mirek & co have drafted something, staged at http://sdo-auto-fix.appspot.com/docs/automotive.html

I'd like to add this under /docs/ alongside our other supporting documentation.

Ping @vholland @scor @nicolastorzec @rvguha @tmarshbing @tilid - can you take a look?

@tmarshbing
Copy link

@tmarshbing tmarshbing commented Jul 17, 2017

This is very comprehensive. I like it! A few suggestions:

  1. Add a link to the examples at the top. I imagine most readers will be most interested in the examples, so let's get them there quickly.
  2. In the hybrid car example, shouldn't we use value instead of maxValue for the weightTotal and wheelbase attributes? It also might be nice to show the kWh for the electric engine. Finally, the pre-markup example is missing the offer that is in the markup.
@sopekmir
Copy link
Contributor

@sopekmir sopekmir commented Jul 19, 2017

Thanks! We will consider adding a line with in-page links to all the sections (we have 7 of them).
Your remarks about hybrid car example will also be taken care of!

@trypuz
Copy link
Contributor

@trypuz trypuz commented Jul 19, 2017

@tmarshbing, thank you for your suggestions. Almost all of them have been applied (see http://sdo-auto-fix.appspot.com/docs/automotive.html), i.e.:

  • we added a table of contents to the document (as @sopekmir suggested)
  • we used value instead of maxValue for the wheelbase property
  • we added description "Battery Energy 1.56 kWh" to the example (please note that in our example we used Ah to describe the battery capacity - see fuelCapacity property)
  • we added the offer info to the pre-markup example

I think it still makes sense to use maxValue for the weightTotal property.

@tmarshbing, would you say that the standard for measuring battery capacity is Watt Hours or Kilo-Watt Hours not of Amp Hours?

@tmarshbing
Copy link

@tmarshbing tmarshbing commented Jul 20, 2017

Thanks for the changes. A few things:

@sopekmir
Copy link
Contributor

@sopekmir sopekmir commented Sep 6, 2017

Oops, @tmarshbing - thanks for the note and sorry - for quite a long time we did not correct the documentation's Example 2, where we still have maxValue. We will do it asap !

@trypuz
Copy link
Contributor

@trypuz trypuz commented Sep 7, 2017

@sopekmir, @tmarshbing as far as the change for "wheelbase" is concerned -- my fault; I've changed maxValule to value only in the example that is here: http://auto.sdo-auto-fix.appspot.com/wheelbase and completely forgot to update the documentation. Should be OK now.

@VladimirAlexiev
Copy link

@VladimirAlexiev VladimirAlexiev commented Sep 20, 2017

A very useful doc! Suggestions for improvement:

  1. The diagram makes it look that all the props apply to Car but in fact they apply to Vehicle. So rename the central node to Vehicle
  2. The first one is a class diagram (very different from the second one which is a property diagram): mark it so, and I’d use a different style (eg a tree) to emphasize that
  3. vehicleEngine should show the class holding the “subsidiary” properties: EngineSpecification
  4. Can you put some dot on the branches showing enumerations: vehicleSpecialUse, driveWheelConfiguration, steeringPosition

In the list below the diagram (The complete hierarchy of all terms in the automotive extension):

  1. Use violet instead of red for the “auto” terms, to match the color on the diagram
  2. Use black instead of brown for auto terms in core
  3. Include section “Enumerated values”

Further:

  1. http://schema.org/vehicleModelDate has no comment. This is not ok, especially given that there is also http://auto.schema.org/modelDate . Explain and contrast the two
  2. Give example of using both props
  3. The TOC is very useful, but also include subheadings in the TOC (the doc is fairly long, so that will ease navigation)
  4. The phrase “Drive & Wheel Configuration” is wrong. It’s "Drive-wheel Configuration" (which wheels drive the car)
  5. In EXAMPLE 2, PRE-MARKUP and JSON-LD Markup don’t correspond (eg PRE-MARKUP doesn’t have productionDate, modelDate, cargoVolume; and "Last updated on" is not the same as availabilityStarts). Either make them the same, or remove PRE-MARKUP.
@trypuz
Copy link
Contributor

@trypuz trypuz commented Sep 21, 2017

@VladimirAlexiev thank you for your detailed and helpful comments! Please find below my answers to all your suggestions.

Ad 1 and 2: We did intentionally picture (in the first two diagrams) the types and properties this way. The only purpose of the two presented mind maps is to show the whole conceptual repertoire of types, properties, and enumerations that can be used to mark up automobiles. So I would leave them as they are.
Ad 3: @VladimirAlexiev could you please explain what you meant here?
Ad 4: Dotted arrows represent the instance-of relation. So there are dots on the branches showing enumerations. Did you mean something different?
Ad 5, 6 and 7: We've changed the colors of core and auto terms to make their look-and-feel compatible with the full type hierarchy "Core plus all extensions" doc. And since the list is shaped in such a way that it illustrates hierarchy (is-a), we decided not to list enumerations in the same way.
Ad 8 and 9: In #1650 and #1674 we have discussed the properties: http://schema.org/vehicleModelDate and http://auto.schema.org/modelDate. In the examples, in our doc, we have only used modelDate.
Ad 10: Subheadings have been added to the TOC.
Ad 11: Corrected.
Ad 12: Missing elements have been added to PRE-MARKUP.

@VladimirAlexiev, let us know if there is something more to be corrected/improved, please.

@VladimirAlexiev
Copy link

@VladimirAlexiev VladimirAlexiev commented Sep 22, 2017

@trypuz @sopekmir
ad 1. Why the prop diagram has Car in the center? Should be Vehicle

ad 2. Could you make the two disconnected parts of the diagram look different, since they mean different things?

ad 3. vehicleEngine leads to a bag of props. That bag has a class (EngineSpecification) that should be shown on the diagram

ad 4. I don't see dotted arrows? The visual from vehicleSpecialUsage to DrivingSchoolVehicleUsage is just the same as from vehicleEngine to fuelType, but the meaning is different.

ad 7. I'm suggesting you include a section “Enumerated values”, after sections "Types" and "Properties"

ad 8. and 9. Then vehicleModelDate should be killed in favor of modelDate: #1746

ad 13. Would be nice to validate all examples, eg:

parsing Error! mismatched tag: line 8, column 6

The problem is unclosed <meta> tags

  • tried to convert Example3 JSONLD to Turtle using Jena RIOT and got this:
riot --formatted=turtle auto-eg3-hybridCar.jsonld  1>auto-eg3-hybridCar.ttl
java.lang.IllegalArgumentException: Illegal character in path at index 8: internal combustion engine
  • rdf-translator gives a better error message:
"file:///base/data/home/apps/s%7Erdf-translator/1.380697414950152317/internal combustion engine"
does not look like a valid URI, I cannot serialize this as N3/Turtle. Perhaps you wanted to urlencode it?
  • The reason is that engineType is declared a URL in the schema.org context (bug in the context: #1747), so you can't use a string.

ad 14. Try to convert each variant to turtle and ensure they produce the same turtle

@trypuz
Copy link
Contributor

@trypuz trypuz commented Sep 22, 2017

Ad 13: Examples have been corrected and validated by https://search.google.com/structured-data/testing-tool. There were 3 problems. There was "," after ""vehicleSpecialUsage":"DrivingSchoolVehicleUsage"" in the example 3. In the example 1 (in rdfa and microdata) there was used the old version of numberOfAirbags prop, i.e. airbags. Thanks for noticing!

@trypuz
Copy link
Contributor

@trypuz trypuz commented Sep 22, 2017

Ad 1, 2, 3, 4: @VladimirAlexiev as I wrote yesterday - I do not think it is a good idea to change the two mind maps (but let us consult @sdml and @sopekmir in this matter). The different types of arrows have been used in section 5 "Basic models".

Ad 8 and 9: You do have my support!

@VladimirAlexiev
Copy link

@VladimirAlexiev VladimirAlexiev commented Sep 22, 2017

ad 13 See #1747, #1748, which are bugs in schema or its jsonld context

@VladimirAlexiev
Copy link

@VladimirAlexiev VladimirAlexiev commented Sep 22, 2017

@trypuz #1749 also affects you.
So "validated by https://search.google.com/structured-data/testing-tool" is not enough. You also need to look at the data in a different format (see 14 above) and eyeball it to ensure the values make sense.

@trypuz
Copy link
Contributor

@trypuz trypuz commented Sep 22, 2017

@VladimirAlexiev wrote:

  • "maybe unitText "AMH" should be unitCode?" - YES
  • vehicleInteriorColor "beige interior" should be just "beige" - YES
    Changed. Thanks!
@VladimirAlexiev
Copy link

@VladimirAlexiev VladimirAlexiev commented Sep 22, 2017

https://gist.github.com/VladimirAlexiev/3415c0e8af9f1b2aaf82c7a43885436c :
auto-eg3-hybridCar-fixed.jsonld fixes the Example3 problems listed above, except that unitCode is screwed up to URLs (due to #1747).
auto-eg3-hybridCar-fixed-fixed.ttl fixes that too, and I think it's a good idea to publish Turtle as well

@VladimirAlexiev
Copy link

@VladimirAlexiev VladimirAlexiev commented Sep 22, 2017

Example 3 http://sdo-auto-fix.appspot.com/docs/automotive.html#used_car_with_damages needs this due to #1749

  "vehicleSpecialUsage": {"@id":"http://schema.org/DrivingSchoolVehicleUsage"},
@VladimirAlexiev
Copy link

@VladimirAlexiev VladimirAlexiev commented Sep 22, 2017

ad 15. http://sdo-auto-fix.appspot.com/docs/automotive.html#rental_car_acriss code:

  • needs this, see http://schema.org/additionalProperty Example1
    "additionalProperty":{ ..., "value":"true"}
  • use URL for vehicleTransmission:
    "vehicleTransmission": "http://auto-types.org/transmission/AutomaticGearbox",

I've added files with such fixes to the above gist

@sopekmir
Copy link
Contributor

@sopekmir sopekmir commented Apr 26, 2018

To all watching this thread: As far as my awerness tells - the documentation of auto.schema.org ( http://sdo-auto-fix.appspot.com/docs/automotive.html ) is in a state that allows it to be included into the upcoming release. All relevant remarks were done (as reported above). If there is anything we would need to do to the document, please let us know.

@VladimirAlexiev
Copy link

@VladimirAlexiev VladimirAlexiev commented Apr 26, 2018

Remaining stuff from the items I posted:

  • ad 1 put Vehicle in the center
  • ad 3 & 4 show classes EngineSpecification, SteeringPositionValue, CarUsageType on the first diagram
  • ad 6 not important
  • ad 7 the enumeration values are described in the running text. But PLEASE add a list Enumerated Values at http://sdo-auto-fix.appspot.com/docs/automotive.html#hierarchy_of_terms, right after the lists of Types and Properties
  • ad 8 & 9: http://auto.schema.org/modelDate has the same description as http://schema.org/vehicleModelDate. One is superfluous and must be REMOVED. #1746 asks to remove vehicleModelDate, and #1677 confirms your intention was to document modelDate. So please remove vehicleModelDate from your documentation
  • ad 10: add 3 subheadings to Hierarchy of terms: Types, Properties, Enumerated Values, so these appear in the TOC
  • ad 13: The same error appears and I tested more:
    • ex1 rdfa: parsing Error! mismatched tag: line 8, column 6
    • ex1 jsonld: Invalid control character at: line 14 column 135 (char 540)
    • ex2 jsonld all these resolve to bad URLs (eg file:///base/data/home/apps/s%7Erdf-translator/2.408516547054015808/electricMotor) due to wrong context:
      unitCode "SEC", bodyType "hatchback", unitCode "FTQ", engineType "electricMotor", engineType "internalCombustionEngine", fuelType "petrol", "automaticGearbox"
    • ex2 steeringPosition "LeftHandDriving" is literal, should be URL
    • ex3 jsonld: "DrivingSchoolVehicleUsage" and "RearWheelDriveConfiguration" are emitted as literal, should be URL
    • ex4 jsonld (due to wrong context): "file:///base/data/home/apps/s%7Erdf-translator/2.408516547054015808/automatic gearbox" does not look like a valid URI
    • ex4 schema:additionalProperty with schema:name "air conditioning" also requires schema:value schema:True
    • ex4 (due to wrong context) schema:unitCode resolves to file:///base/data/home/apps/s%7Erdf-translator/2.408516547054015808/DAY
  • ad 16 (new one): please add Turtle examples from my gist

Thanks for fixing the rest!

@RichardWallis
Copy link
Contributor

@RichardWallis RichardWallis commented Apr 26, 2018

The prerelease version of the documentation can be viewed here: http://webschemas.org/docs/automotive.html - expected to be part of v3.4

Be aware that any future changes should be referenced against the version currently part of the master branch of https://github.com/schemaorg/schemaorg

@sopekmir
Copy link
Contributor

@sopekmir sopekmir commented Apr 26, 2018

@RichardWallis - thanks. Good news! @VladimirAlexiev I have asked Robert @trypuz to answer you precisely.

Let me only recall that we made all really needed changes already and Robert reported them here.
For the remaining remarks coming from you, we decided not to reflect them in the document at this stage.

@trypuz
Copy link
Contributor

@trypuz trypuz commented Apr 27, 2018

@VladimirAlexiev - thank you for your comments and suggestions.

  • ad 1: Discussed above.
  • ad 3 & 4: Will be done soon.
  • ad 7: Done.
  • ad 8 & 9: Discused above.
  • ad 10: Done.
  • ad 13:
  • ex1 rdfa: parsing Error! mismatched tag: line 8, column 6: Done.
  • ex1 jsonld: Invalid control character at: line 14 column 135 (char 540): Done.
  • ex2 steeringPosition "LeftHandDriving" is literal, should be URL: Done.
  • ex3 jsonld: "DrivingSchoolVehicleUsage" and "RearWheelDriveConfiguration" are emitted as literal, should be URL: Done.
  • ad 16: See no need for that.

Changes implemented. See: http://sdo-auto-fix.appspot.com/docs/automotive.html
Resolving issues considering ex2 and ex4 are under investigation.

@RichardWallis
Copy link
Contributor

@RichardWallis RichardWallis commented Apr 27, 2018

@trypuz Good to see progress here.

When the 'Will be done soon' has been done, can you create a PR to enable the merge of these updates int the master branch?

@sdml
Copy link

@sdml sdml commented Apr 27, 2018

We've added EngineSpecification and enumeration types to the diagram.

trypuz added a commit to trypuz/schemaorg that referenced this issue Apr 27, 2018
@trypuz
Copy link
Contributor

@trypuz trypuz commented Apr 27, 2018

@RichardWallis - PR created.

@VladimirAlexiev
Copy link

@VladimirAlexiev VladimirAlexiev commented Apr 30, 2018

ex2 rdfa: #1906.
ad8, ad9: do you mean that #1650 and #1674 need to be fixed? Agree; then @RichardWallis it seems to me those need to be fixed asap to avoid confusion when the Auto extension is published.

danbri added a commit that referenced this issue May 1, 2018
* Documentation for auto has been fixed (#1677).

* HTML errors corrected
@jmendes84
Copy link

@jmendes84 jmendes84 commented May 22, 2018

Hi, I'm curious if there are plans to incorporate lease pricing into the schema for autos or should that be under car? Lot's of dealer sites have New Car Specials (lease offers) that are advertised and it would be great to be able to add schema to these pages. I hope this is the right place to ask this question. Thanks!

@Dataliberate
Copy link
Contributor

@Dataliberate Dataliberate commented May 22, 2018

@jmendes84
Copy link

@jmendes84 jmendes84 commented May 23, 2018

@github-actions
Copy link

@github-actions github-actions bot commented Jul 21, 2020

This issue is being tagged as Stale due to inactivity.

@danbri
Copy link
Contributor Author

@danbri danbri commented Aug 14, 2020

This was published ages ago - thanks again everyone!

https://schema.org/docs/automotive.html

@danbri danbri closed this Aug 14, 2020
@danbri danbri self-assigned this Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants