fix issue#21 #22
fix issue#21 #22
Conversation
| ''' | ||
|
|
||
| data_dict = {} | ||
| for field_name in transitfeed.Agency._FIELD_NAMES: |
There was a problem hiding this comment.
This variable is protected, we should not access it directly. I'll change this before merging.
| @@ -298,5 +293,34 @@ def load_config(file): | |||
|
|
|||
| return config; | |||
|
|
|||
| def load_agency(config): | |||
There was a problem hiding this comment.
Just a German style nitpick for the future ;) PEP8 requires two blank lines between methods.
| @@ -298,5 +293,34 @@ def load_config(file): | |||
|
|
|||
| return config; | |||
|
|
|||
| def load_agency(config): | |||
| ''' | |||
There was a problem hiding this comment.
Great you are adding documentation already! Docstrings should be made with double quotes " btw.
| data_dict[field_name] = field_value | ||
|
|
||
| except KeyError: | ||
| sys.stderr.write("key '"+field_name+"' was not found on the config file.") |
There was a problem hiding this comment.
I think it should be "in the config file".
There was a problem hiding this comment.
Sometime later, we should maybe distinguish here between required and optional fields or does the validation already print warnings for that?
| if field_value is not None and field_value != "": | ||
| data_dict[field_name] = field_value | ||
|
|
||
| except KeyError: |
There was a problem hiding this comment.
I think it would be better to check if field_name is in config["agency"] rather than catching KeyErrors.
| timezone=config['agency']['agency_timezone'], | ||
| agency_id=config['agency']['agency_id'] | ||
| ) | ||
| agency = load_agency(config) |
There was a problem hiding this comment.
This will break the script, because the agency is never added to the schedule object. Did you test this?
There was a problem hiding this comment.
You are right, I'm sorry. I thought I tested but obviously I make a mistake.
| if not agency.Validate(): | ||
| sys.stderr.write("Agency data has errors") | ||
| # later, an error handling based on a exception here could be added. | ||
| #raise AttributeError('Agency data on config file in not valid.') |
There was a problem hiding this comment.
Maybe we should distinguish here between required and optional fields. Do you know how the validation behaves? Maybe it prints warnings already?
There was a problem hiding this comment.
The validate method returns a boolean and have a mechanism to handle problems. I think the method "load_agency" could be improved to be less naive.
|
Maybe could be more simple, just invoke the Agency constructor like this: agency = transitfeed.Agency(field_dict=config["agency"]) there is no more need of the load_agency method. |
No description provided.