search

arjanadriaanse / pascal-language-server

LSP server implementation for Pascal
GNU General Public License v3.0
56 stars 20 forks source link

optional values in protocol #10

Closed arjanadriaanse closed 4 years ago

arjanadriaanse commented 4 years ago

fixes #7

arjanadriaanse commented 4 years ago

Optional properties can be defined with the type TOptionalVariant<T> for simple types and TOptionalObject<T> for object types. Both types have the properties HasValue: Boolean and Value: T.

genericptr commented 4 years ago

This looks sane to me but it's going to be some ugly boiler plate. I think TOptional needs to be a record also so we can access the value using properties (otherwise TONS of getters/setters will be involed). Classes won't let you do this because it's "unsafe". Stupid restriction IMO and I even fought with the compiler team on it but they wouldn't budge. :)

  TOptionalString = specialize TOptional<String>;

  TThing = class(TPersistent)
  private
    fName: TOptionalString;
  published
    property name: string read fName.value write fName.value;
  end;

Also I'm not sure the variant is going to place nice with properties unless we add runtime checks during streaming. Making a bunch of specializes for various types is probably less work.

arjanadriaanse commented 4 years ago

The problem is that the JSON streaming process can only access published properties and needs to know which ones are not set so it can omit them. With how it is implemented now your example would work as follows.

  TOptionalString = specialize TOptionalVariant<string>;

  TThing = class(TPersistent)
  private
    fName: TOptionalString;
  published
    property name: TOptionalString read fName write fName;
  end;

The value can then be accessed and changed through name.Value, cleared through name.Clear and name.HasValue shows whether or not the value is set. The boilerplate is not that big, because the required getters and setters are already implemented generically.

How could this be implemented with records while still having this functionality through published properties?

genericptr commented 4 years ago

No, you're right actually. Making those records would totally defeat the purpose of making them optionals. ;) I think you have the right idea so if you commit this I'll try implementing across all the various classes I've made so far.

arjanadriaanse commented 4 years ago

Alright, I will try the same. Be sure to let me know if you find that changes or additions are needed.