SpiceSharp / SpiceSharpParser

SPICE netlists parser for .NET
MIT License
25 stars 6 forks source link

Change behavior of the library - trying to not exit on first exception #147

Closed marcin-golebiowski closed 4 years ago

ToddThomson commented 4 years ago

@marcin-golebiowski OK. I've got the changes into my local dev branch.

The first thing I noticed was that the parser return result is providing circuit validation results too. This is likely not a good idea. In my opinion Parsing and Validation are 2 separate concerns. Parsing could very well be successful, but validation could still fail.

I had thought that you were going to add the validation changes to the Circuit class in SpiceSharp. That is where they should go. Please see SpiceSharp/SpiceSharp#97.

ToddThomson commented 4 years ago

@marcin-golebiowski Unless you have a good reason for the use of the generics in:

        public ISpiceModel<Circuit, Simulation> SpiceModel { get; set; }

An ISpiceModel interface without the generics is much less complicated. As a consumer of the API I would much prefer the non-generic version of the interface or SpiceModel.

ToddThomson commented 4 years ago

@marcin-golebiowski Thanks for being so responsive! I have enjoyed the shared experience over the past few days. Have a nice weekend.

ToddThomson commented 4 years ago

@marcin-golebiowski The 2 main things I need for parsing netlists are:

  1. API that returns all parsing errors. An error would contain a message and optionally a separate line # and col #. With that I can display the netlist and then add error formatting by line.
  2. Model validation ( this is what you worked on, but not what I was thinking about - I was looking at circuit validation ) I will review this. Then
  3. Circuit Validation. I don't think that you've done anything yet as I didn't see any commits to SpiceSharp.

Anyway, off the play Friday night squash! It has been a long day.

marcin-golebiowski commented 4 years ago

At the moment library is not very ready to recover from parsing and lexing errors. The PR under review is introducing validation result that contains:

marcin-golebiowski commented 4 years ago

@ToddThomson According to my knowledge circuit validation in SpiceSharp is in development. There is a special branch with version 3.0 where validation is implemented by Sven.

svenboulanger commented 4 years ago

Circuit validation had a basic implementation in 2.x. I can confirm that 3.x will come with a more customizable validation system.

I'd also like to say that it's fun to see you guys actively discussing the development of the parser. 🙂 If I can do anything to help don't hesitate to ask!

ToddThomson commented 4 years ago

My input for the parser or any other component generating "diagnostics" is:

Please, please don't use exceptions where you don't have to. Putting try catch around library API is something I personally do not like. It complicates code; keep your API as simple as possible.

Give me all the diagnostics ( or some category of diagnostics )

Diagnostics should contain: A text message that means something to an end user; A range (from,to) specifying where the issue was found; a diagnostic category; a diagnostic code.

marcin-golebiowski commented 4 years ago

Thanks for the Input. I personally agree. Your request for change will be done after December 30.