SpiceSharp / SpiceSharpParser

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

API Return Results Vs Exceptions #142

Closed ToddThomson closed 3 years ago

ToddThomson commented 4 years ago

It would be nice to have the Parser/Validator API return an object with full file parsing/validation errors ( or competed successfully ) rather than halting at the first exception.

I believe you do this with "Warnings" in the circuit model. (NOTE: do simulations run with warnings? ).

marcin-golebiowski commented 4 years ago

@ToddThomson It will be done in few days. I will let you know when it's ready.

ToddThomson commented 4 years ago

@marcin-golebiowski Thanks Marcin.

Here is what I usually do for services I write. You would need to add properties ( lineNumber, colNumber, etc. ) to the error class. I hope it helps:

    public class SpiceServiceResult
    {
        #region Fields

        private static readonly SpiceServiceResult _success = new SpiceServiceResult( true );

        #endregion

        #region Constructors

        private SpiceServiceResult( bool success )
        {
            Succeeded = success;
        }

        public SpiceServiceResult( SpiceServiceError error )
        {
            Succeeded = false;
            Error = error;
        }

        /// <summary>
        /// Creates a new <see cref="SpiceServiceResult"/> with the provided <see cref="EntityServiceError"/> error.
        /// </summary>
        /// <param name="errorType"></param>
        /// <param name="error"></param>
        public SpiceServiceResult( SpiceServiceErrorType errorType, SpiceServiceError error )
        {
            Succeeded = false;
            ErrorType = errorType;
            Errors = new SpiceServiceErrorCollection();

            Errors.AddError( error );
        }

        /// <summary>
        /// Creates a new <see cref="SpiceServiceResult"/> with the provided <see cref="SpiceServiceErrorCollection"/> errors.
        /// </summary>
        /// <param name="errorType"></param>
        /// <param name="errors"></param>
        public SpiceServiceResult( SpiceServiceErrorType errorType, SpiceServiceErrorCollection errors )
        {
            Succeeded = false;
            ErrorType = errorType;
            Errors = errors;
        }

        #endregion

        #region Methods

        /// <summary>
        /// Creates a failed <see cref="SpiceServiceResult"/> with the <see cref="Exception"/> cause.
        /// </summary>
        /// <param name="errorType"></param>
        /// <param name="e"></param>
        /// <returns></returns>
        public static SpiceServiceResult Failed( SpiceServiceErrorType errorType, Exception e = null )
        {
            return new SpiceServiceResult( errorType, new SpiceServiceError( string.Empty, e ) );
        }

        /// <summary>
        /// Creates a failed <see cref="SpiceServiceResult"/> with the <see cref="EntityServiceError"/> cause.
        /// </summary>
        /// <param name="errorType"></param>
        /// <param name="error"></param>
        /// <returns></returns>
        public static SpiceServiceResult Failed( SpiceServiceErrorType errorType, SpiceServiceError error = null )
        {
            return new SpiceServiceResult( errorType, error );
        }

        /// <summary>
        /// Creates a failed <see cref="SpiceServiceResult"/> with <see cref="SpiceServiceErrorCollection"/>.
        /// </summary>
        /// <param name="errorType"></param>
        /// <param name="errors"></param>
        /// <returns></returns>
        public static SpiceServiceResult Failed( SpiceServiceErrorType errorType, SpiceServiceErrorCollection errors = null )
        {
            return new SpiceServiceResult( errorType, errors );
        }

        #endregion

        #region Properties
        /// <summary>
        /// Gets the service layer errors.
        /// </summary>
        public SpiceServiceErrorCollection Errors { get; private set; }

        /// <summary>
        /// Gets the service layer error type.
        /// </summary>
        public SpiceServiceErrorType ErrorType { get; private set; }

        public SpiceServiceError Error { get; private set; }

        public bool Succeeded { get; private set; }

        public static SpiceServiceResult Success
        {
            get
            {
                return _success;
            }
        }

        #endregion
    }
    public class SpiceServiceError
    {
        #region Constructor(s)

        /// <summary>
        /// Creates a new SpiceServiceError with the specific <see cref="Exception"/> cause.
        /// </summary>
        /// <param name="key"></param>
        /// <param name="exception"></param>
        public SpiceServiceError( string key, Exception exception )
            : this( key, exception, errorMessage: null )
        {
            if ( key == null )
            {
                throw new ArgumentNullException( nameof( key ) );
            }

            if ( exception == null )
            {
                throw new ArgumentNullException( nameof( exception ) );
            }
        }

        /// <summary>
        /// Creates a new <see cref="EntityServiceError"/> with the specific <see cref="Exception"/> and error message.
        /// </summary>
        /// <param name="key"></param>
        /// <param name="exception"></param>
        /// <param name="errorMessage"></param>
        public SpiceServiceError( string key, Exception exception, string errorMessage )
            : this( key, errorMessage )
        {
            Exception = exception ?? throw new ArgumentNullException( nameof( exception ) );
        }

        /// <summary>
        /// Creates a new <see cref="EntityServiceError"/> with the specific error message.
        /// </summary>
        /// <param name="key"></param>
        /// <param name="errorMessage"></param>
        public SpiceServiceError( string key, string errorMessage )
        {
            Key = key ?? throw new ArgumentNullException( nameof( key ) );

            ErrorMessage = errorMessage ?? string.Empty;
        }

        #endregion

        #region Properties

        /// <summary>
        /// 
        /// </summary>
        public string Key { get; }

        /// <summary>
        /// Gets the service error exception cause.
        /// </summary>
        public Exception Exception { get; }

        /// <summary>
        /// Gets the service error message.
        /// </summary>
        public string ErrorMessage { get; }

        #endregion
    }
public class SpiceServiceErrorCollection : IEnumerable<SpiceServiceError>
{
    private List<SpiceServiceError> _errors = new List<SpiceServiceError>();

    /// <summary>
    /// Gets the read only service error collection.
    /// </summary>
    public IReadOnlyCollection<SpiceServiceError> Errors => new ReadOnlyCollection<SpiceServiceError>( _errors );

    /// <summary>
    /// Adds a <see cref="SpiceServiceError"/> to the error collection.
    /// </summary>
    /// <param name="error"></param>
    public void AddError( SpiceServiceError error )
    {
        _errors.Add( error );
    }

    /// <summary>
    /// Adds a new <see cref="SpiceServiceError"/> to the error collection from a key/exception pair.
    /// </summary>
    /// <param name="key"></param>
    /// <param name="exception"></param>
    public void AddError( string key, Exception exception )
    {
        if ( key == null )
        {
            throw new ArgumentNullException( nameof( key ) );
        }

        if ( exception == null )
        {
            throw new ArgumentNullException( nameof( exception ) );
        }

        _errors.Add( new SpiceServiceError( key, exception ) );
    }

    /// <summary>
    /// Adds a new <see cref="EntityServiceError"/> to the collection from the key, exception and message parameters.
    /// </summary>
    /// <param name="key"></param>
    /// <param name="exception"></param>
    /// <param name="errorMessage"></param>
    public void AddError( string key, Exception exception, string errorMessage )
    {
        if ( key == null )
        {
            throw new ArgumentNullException( nameof( key ) );
        }

        if ( exception == null )
        {
            throw new ArgumentNullException( nameof( exception ) );
        }

        if ( errorMessage == null )
        {
            throw new ArgumentNullException( nameof( errorMessage ) );
        }

        _errors.Add( new SpiceServiceError( key, exception ) );
    }

    /// <summary>
    /// Adds a new <see cref="EntityServiceError"/> to the collection from the key and message parameters.
    /// </summary>
    /// <param name="key"></param>
    /// <param name="errorMessage"></param>
    public void AddError( string key, string errorMessage )
    {
        if ( key == null )
        {
            throw new ArgumentNullException( nameof( key ) );
        }

        if ( errorMessage == null )
        {
            throw new ArgumentNullException( nameof( errorMessage ) );
        }

        _errors.Add( new SpiceServiceError( key, errorMessage ) );
    }

    /// <inheritdoc />
    public IEnumerator<SpiceServiceError> GetEnumerator()
    {
        return Errors.GetEnumerator();
    }

    /// <inheritdoc />
    IEnumerator IEnumerable.GetEnumerator()
    {
        return Errors.GetEnumerator();
    }
}
public enum SpiceServiceErrorType : int
{
    /// <summary>
    /// Indicates an unknown error type.
    /// </summary>
    Unknown = 0x01,

    /// <summary>
    /// Indicates entity validation error type.
    /// </summary>
    Validation = 0x02,

    /// <summary>
    /// Indicates a repository error type.
    /// </summary>
    Parsing = 0x03,
}
marcin-golebiowski commented 4 years ago

@ToddThomson If you prefer/want feel free to open PR or I will use your code in the repository.

ToddThomson commented 4 years ago

@marcin-golebiowski Go ahead and use what you can to speed things along. I am busy working through the simulator code now.

marcin-golebiowski commented 4 years ago

@ToddThomson Just to let you know about ETA for this: 31 December

ToddThomson commented 4 years ago

@marcin-golebiowski Hey, we all need holiday time! I look forward to seeing the changes.

marcin-golebiowski commented 4 years ago

@ToddThomson I will try to implement changes today but I'm not sure if I make it.

marcin-golebiowski commented 4 years ago

Please review #147. It's what I was able to do in short time. If it's ok, I will publish nuget package with it,

ToddThomson commented 4 years ago

@marcin-golebiowski Deployment to NuGet is not really necessary. I use VS2019 GitHub services to pull your changes into my local repo. It is also likely a good idea to limit the changes to the released Nuget package version. Perhaps a dev branch would be in order? When working on a project (always in debug mode) that is in development stage I use the following within csproj:

<Choose>
    <When Condition="'$(Configuration)' == 'Debug'">
      <ItemGroup>
        <ProjectReference Include="..\..\SpiceSharp\SpiceSharp\SpiceSharp.csproj" />
      </ItemGroup>
    </When>
    <Otherwise>
      <ItemGroup>
        <PackageReference Include="SpiceSharp" Version="2.8.0" />
      </ItemGroup>
    </Otherwise>
  </Choose>
ToddThomson commented 4 years ago

If you create a dev branch for all your SpiceSharp repos I will hook up my VS2019 to use them ( rather than master ).

marcin-golebiowski commented 4 years ago

Development branch has been created.

I thought that Nuget packages every few days were good idea. They were backward compatible so new features where added in new versions.

ToddThomson commented 4 years ago

Yeah, no issues with creating NuGet packages that don't change the API. Better to push to Master when you know you are not going to break customer apps that depend on your library!

marcin-golebiowski commented 4 years ago

OK. I will use development branch from today

marcin-golebiowski commented 4 years ago

@ToddThomson 1.5.0 version available on Nuget contains requested changes

ToddThomson commented 4 years ago

@marcin-golebiowski , @svenboulanger Thank-you. I will incorporate your changes this coming week.

I have made progress on the Spice# Studio app. UWP is an interesting challenge given its WinRT beginnings. However, MS is correcting its mistakes and with Win UI 3 coming this year, UWP will be equipped to build desktop apps.

My main challenge was to build a page navigation service to support multiple windows. The Spice# Studio app supports 3 main panes: LHS Main/Solution tool panes based on the standard UWP Navigation View; Central Tabbed documents pane; RHS tool pane based on a custom control which utilizes the Navigation view. There is also a main top menu above a command toolbar and a lower status bar.

I should be able to get a beta pushed up to Github in the next 2 to 3 weeks.

ToddThomson commented 4 years ago

@svenboulanger I would like to be able to run the parser on an async background thread as the user is actively editing the netlist. Do you have any plans for providing an async API?

marcin-golebiowski commented 4 years ago

@ToddThomson On SpiceSharpParser level I could provide some async API for simulation that uses events to deliver async result.

ToddThomson commented 4 years ago

@marcin-golebiowski @svenboulanger The SpiceParserValidationResult looks good! Thank-you.

The Spice# Studio app has a service for interaction with the Spice# library modules. It has a BuildAsync( netlist, warningsAsErrors ) method. Internally the builder calls ParseNetList() and then if no errors are encountered, calls model.Circuit.Validate();

The circuit Validate() call doesn't return anything other that an exception if there is a problem. Was this going to change? A SpiceModelValidationResult or SpiceCircuitValidationResult would seem appropriate. I can't recall if this was being worked on or not. Please let me know.

ToddThomson commented 4 years ago

@svenboulanger To determine if a validation error or warning occurred during the ParseNetlist() call, one needs to check each Validation Source...

        public SpiceParserValidationResult()
        {
            Lexing = new LexerValidationResult();
            Parsing = new ParsingValidationResult();
            Reading = new SpiceNetlistValidationResult();
        }

Given that a ValidationEntry has a Source property, it is not necessary to have individual collections. It might be better to have a single collection and then properties to return All and filtered results for the individual Sources.

svenboulanger commented 4 years ago

Hi @ToddThomson ,

I would like to be able to run the parser on an async background thread as the user is actively editing the netlist. Do you have any plans for providing an async API?

I am not planning on adding an async API for Spice# (core). You can always run Spice# simulations in a background thread if you don't wan't the UI to freeze. Spice# v3 will have support for multithreaded computations, but that is really only meant for performance optimizations, and is usually very dependent on the circuit.

The circuit Validate() call doesn't return anything other that an exception if there is a problem. Was this going to change?

Currently, Spice# v3 validation will be done by running a set of rules on subjects. After validation completes, all rule violations can be enumerated (and therefore be filtered, logged, etc.). Additionally, a simulation runs its own set of strictly necessary rules (if rule checking for that simulation is not disabled), and will throw an exception if there are rule violations. I did not really want to distinguish between errors and warnings for rules because that would be context-dependent.

Summarized I would describe the validation system as: IRules contains multiple IRule definitions. You can feed these rules IRuleSubject instances (usually circuit components) that each IRule can use to find rule violations. After you have fed all subjects to the rules, you can enumerate all IRuleViolations that each specify the rule that was violated, and optionally the IRuleSubject that caused the rule violation.

ToddThomson commented 4 years ago

@svenboulanger Yeah, don't worry about the async API. I will add the async calls as needed into the SpiceSharp service layer.

I look forward to seeing the new features in Spice# v3!

marcin-golebiowski commented 4 years ago

@ToddThomson I removed derived validation collection classes #150. It's not yet merged into master. Please let me know what you think.

ToddThomson commented 4 years ago

@marcin-golebiowski I'll be able to take a look next week at some point. Thanks!

marcin-golebiowski commented 3 years ago

Changed by #152.

The validation result collection was simplified.