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

Missing base class for all GTFS exceptions #27

Closed
rorlic opened this issue Jan 19, 2016 · 2 comments
Closed

Missing base class for all GTFS exceptions #27

rorlic opened this issue Jan 19, 2016 · 2 comments

Comments

@rorlic
Copy link

rorlic commented Jan 19, 2016

The GTFS exceptions in namespace GTFS.Exceptions all derive from the System.Exception class.

In order to log all issues that occur while reading a GTFS feed, one now needs to either catch the base class System.Exception or catch all exceptions from namespace GTFS.Exceptions explicitly.

Catching System.Exception is considered bad practice. The second approach is also not ideal because if a new GTFS exception is added, it will pass through.

IMHO, it is best to add an abstract base class (e.g. class GTFSExceptionBase) for all GTFS exceptions in namespace GTFS.Exceptions.

Ugly workaround for now:

        try
        {
            gtfsData.GtfsFeed = reader.Read(gtfsDirectorySource);
        }
        catch (Exception ex)
        {
            var type = ex.GetType();
            if (!type.FullName.StartsWith("GTFS.Exceptions"))
            {
                throw;
            }

            // TODO: add logging here
        }
@xivk
Copy link
Contributor

xivk commented Jan 19, 2016

Two things:

  • The feed validation process needs to be reworked.
  • Exceptions should only occur when reading the feed is impossible.

I agree that it would be better to have a base class. Feel free to submit a pull request, otherwise I'll try and do this next time I work on this.

@rorlic
Copy link
Author

rorlic commented Feb 10, 2016

Added class GTFSExceptionBase, derived all exceptions currently in namespace GTFS.Exceptions from it and added unit test to ensure all exceptions derive from this base.

rorlic pushed a commit to rorlic/GTFS that referenced this issue Feb 10, 2016
xivk added a commit that referenced this issue Feb 17, 2016
@xivk xivk closed this as completed Feb 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants