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

Additional locations #58

Closed
forslund opened this issue Feb 1, 2016 · 7 comments
Closed

Additional locations #58

forslund opened this issue Feb 1, 2016 · 7 comments
Milestone

Comments

@forslund
Copy link
Collaborator

forslund commented Feb 1, 2016

Some caches provides a list of additional waypoints (GC58QHM, GCMNF7).

The additional waypoints are commonly used for adding coordinates for Parking areas, steps in multi caches and similar things.

The additional waypoints list contains the following columns for each waypoint

  • Visible
  • Type
  • Prefix
  • Lookup
  • Name
  • Coordinate
  • Note

My suggestion is that a light object is created (perhaps a namedtuple) with at least Type, Lookup, Name, Coordinate and Note fields.

Either the list is converted to a dict with the Lookup (name is not unique) as key or as a plain list.

I will submit a pull request referencing this issue with a simple implementation.

@tomasbedrich
Copy link
Owner

Okay, this looks like a good idea.

I just have to think, where to put the parsing code? Do you think, it would be more meaningful to put it in a complex loop to Cache class? Or something like from_html method in a new Waypoint class + a simpler loop during the cache loading? It is kind of philosophical question...

@forslund
Copy link
Collaborator Author

forslund commented Feb 5, 2016

Had some trouble rebasing the my branch. My initial thought was that yet another class would be a bit of an overkill but after trying to implement this it might be better to have a class instead of a namedtuple. The parsing loop is a bit tricky since two lines need to be parsed for each entry, I would definitely split the parsing from the main load() method since it is complex enough as it is.

This first suggestion is how I personally would do it, a function returning a dict is ok since it doesn't need any persistent state stored in Cache object but it breaks the coding style a bit.

A separate class might be the best solution?

@tomasbedrich
Copy link
Owner

First of all, please unify the naming. I think Waypoint is better than AdditionalLocation, because it is brief and straightforward. Regarding the naming – there are also some misuses of "waypoint" word in the docstrings, it would be great to update them to something like "GC codes", "waypoint codes" or something similar.

Yes, the function returning a dict is a good solution, but current code location is really unfortunate. I would prefer creating a new class (Waypoint) with a class method parsing the waypoints into dict (exactly like you did). Something in style of cache.Size.from_string, but returning a dict of instances. In the final, the Waypoint class would be just smarter namedtuple with one class method.

@forslund
Copy link
Collaborator Author

Ok, I'll look into it during the weekend.

@forslund
Copy link
Collaborator Author

I've done the changes you suggested and changed waypoint -> GC code in cache.py. Is this what you intended or did I misunderstand something?

Should I add a str and repr to the class or is it ok to have a "bare bones" class?

Test aren't included yet.

@tomasbedrich
Copy link
Owner

It looks good. Just a few more things:

  • Documentation for Cache.waypoints property – please add a :type.
  • Documentation for Waypoint class properties – it would be better to have it in the style of Cache class, because of nicer documentation rendering.
  • The tests, as you said.
  • Optionaly str or repr methods – I will leave it completly up to you, what do you think it will be more useful.

@tomasbedrich tomasbedrich added this to the 3.5 milestone Mar 26, 2016
@tomasbedrich
Copy link
Owner

I have just merged #59 and will release the features in version 3.5. Thank you Åke and happy easter to you too!

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