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 VirtualLocation type #203

Closed
pinkplus opened this issue Sep 8, 2020 · 14 comments · Fixed by #302
Closed

Add VirtualLocation type #203

pinkplus opened this issue Sep 8, 2020 · 14 comments · Fixed by #302
Labels
enhancement Issues describing an enhancement or pull requests adding an enhancement. help wanted Help wanted from the community.

Comments

@pinkplus
Copy link

pinkplus commented Sep 8, 2020

Describe the feature

It looks that the VirtualLocation type is added to schema.org recently. Can we get it added to the library?

Schema Objects

Which schema.org object is this about if any?

https://schema.org/VirtualLocation

@pinkplus pinkplus added the enhancement Issues describing an enhancement or pull requests adding an enhancement. label Sep 8, 2020
@RehanSaeed
Copy link
Owner

From schema.org:

This term is proposed for full integration into Schema.org, pending implementation feedback and adoption from applications and websites.

We don't generate code for pending types at the moment.

@pinkplus
Copy link
Author

pinkplus commented Sep 8, 2020

OK, I understand it's still a pending type.

Google says it wants us to use the VirtualLocation type for online events though. From Google's doc:

You can start using the VirtualLocation type even though it's still pending on schema.org.

Is there a workaround that I can use?

@RehanSaeed
Copy link
Owner

Wow, ok. I can see why that is a major problem.

I've thought about adding all Pending types in the past but the assembly would get much larger with so many extra types and there would be a lot of breaking changes between versions.

If you want a solution right now, we support creating your own custom types using inheritance.

I'm not sure about a longer term solution just yet. I'd want to check the schema.org git repo to see what's going on with this type. It may be that it's just about to be officially released. Then we just need to wait a bit.

@RehanSaeed RehanSaeed reopened this Sep 8, 2020
@pinkplus
Copy link
Author

pinkplus commented Sep 8, 2020

Yeah, let me try inheriting for now.

Regarding the long term solution, what if we can put the pending types into a separate assembly? The main assembly size won't be so big. People can get the pending assembly only when they need it. Not sure about breaking changes though... Maybe we have to say that the types can different because they are not finalized.

@danhickman
Copy link

First off, thanks for the library. Big time saver. I would also like to see https://schema.org/VirtualLocation and the related https://schema.org/eventAttendanceMode property added ASAP. This has quickly become important because of the COVID-19 situation so a quick response to adding these would be appreciated.

As for creating a custom type inheritance, in order to use a custom VirtualLocation as the Event.location property, what class would it need to inherit from? Does anything custom need to be implemented to ensure the serialization will be performed correctly on the custom class? Do you have an example of this?

@Turnerj
Copy link
Collaborator

Turnerj commented Sep 19, 2020

@pinkplus : Regarding the long term solution, what if we can put the pending types into a separate assembly? The main assembly size won't be so big. People can get the pending assembly only when they need it. Not sure about breaking changes though... Maybe we have to say that the types can different because they are not finalized.

I do like the idea of pending types in a separate assembly as it both allows the use of them but also indicates that they are still pending so things might change. We'd have to do a bit of work with how our tooling generates the classes to define that split though which is a bit of a big project. There are plans to do some larger work to the tooling (moving to C# Source Generators) which may make this process a lot easier though that is still a while away.

That said, the next bit is important in your case too...

@danhickman : As for creating a custom type inheritance, in order to use a custom VirtualLocation as the Event.location property, what class would it need to inherit from? Does anything custom need to be implemented to ensure the serialization will be performed correctly on the custom class? Do you have an example of this?

There is technically support for inherited types though depending on how you use it. I think you will have issues with VirtualLocation though as Event.Location supports only these types: IPlace, IPostalAddress, string. You don't have control of that. This can be one of the issues with trying to add support for pending types when they introduce changes to existing types.

So to use a custom VirtualLocation on that property, you'd need to unfortunately implement IPlace (and most likely inherit Place unless you want to implement all the properties yourself). After getting past that hurdle, make sure to decorate your custom properties like so (taken from Place as an example):

/// <summary>
/// The fax number.
/// </summary>
[DataMember(Name = "faxNumber", Order = 114)]
[JsonConverter(typeof(ValuesJsonConverter))]
public OneOrMany<string> FaxNumber { get; set; }

/// <summary>
/// The geo coordinates of the place.
/// </summary>
[DataMember(Name = "geo", Order = 115)]
[JsonConverter(typeof(ValuesJsonConverter))]
public Values<IGeoCoordinates, IGeoShape> Geo { get; set; }

DataMember is used to specify the property name during serialization. The order shouldn't be necessary on your end (and we will actually lose the order once we've moved to System.Text.Json). However the JsonConverter is extremely important - you need to specify this so we can convert to and from the various different properties for you.

With all this done, creating your custom VirtualLocation and setting it on the Event.Location property should work fine for serialization.

Deserialization on the other hand is a much more complicated story. Have a look at the thread from this comment down to understand what some of our issues are with deserialization. The short version is: We can't easily determine the deserialization target from the "@type" property for custom types.

@RehanSaeed
Copy link
Owner

I'd be happy with a new Schema.NET.Pending NuGet package containing all the pending types. I'd be happy to take a PR for that work.

The only major downside I can see is that the pending types are liable to change a lot and so we'd have to push out package updates quite frequently to keep up. I'm not sure that we'd do it often enough.

It's probably also worth you pushing for the types you want to be moved from pending to release in the upstream schema.org project itself. There's probably an open issue about these types already. Especially, if Google use these types.

@RehanSaeed RehanSaeed added the help wanted Help wanted from the community. label Nov 2, 2020
@cduivis
Copy link

cduivis commented Nov 18, 2020

@RehanSaeed did you already create a branch for the pending types to which PR's could be opened for pending types that we would want to use?

@RehanSaeed
Copy link
Owner

No, not yet. I'd be happy to take a PR for it.

@danhickman
Copy link

@RehanSaeed can we move forward on the separate Schema.NET.Pending package ASAP? This is really needed by the community.

@RehanSaeed
Copy link
Owner

I don't currently have bandwidth to work on this feature. However, I'd be willing to take a PR that creates a new Schema.NET.Pending project and modifies Schema.NET.Tool to spit out all pending types into that project. As @Turnerj talks about, I think we should avoid adding pending properties to production/released types.

@Turnerj
Copy link
Collaborator

Turnerj commented Jun 25, 2021

Hey @pinkplus , @danhickman , @cduivis - I know it has been a while since this was discussed but I wanted to let you know that we've released Schema.NET.Pending on NuGet now which has all the properties and types (both pending and not). You can reference this instead of Schema.NET.

@danhickman
Copy link

@Turnerj much appreciated. I'll be utilizing it for sure.

@pinkplus
Copy link
Author

Thanks @Turnerj!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues describing an enhancement or pull requests adding an enhancement. help wanted Help wanted from the community.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants