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.

Sign up for GitHub

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

Jump to bottom

Separate NMEASentence values collection #114

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Separate NMEASentence values collection #114

wants to merge 6 commits into from

Conversation

ptoews
Copy link

@ptoews ptoews commented Jun 30, 2020 •
edited

Started to resolve #107.
Separated the value collection into two methods, one for named values (by fields) returning a dict and another one without names returning just a list. Also integrated them into __repr__.
Here is how they work currently:

msg = pynmea2.parse("$GPGGA,184353.07,1929.045,S,02410.506,E,1,04,2.6,100.00,M,-33.9,M,,0000*6D")
msg.field_values()
Out[9]: 
{'timestamp': datetime.time(18, 43, 53, 70000),
 'lat': '1929.045',
 'lat_dir': 'S',
 'lon': '02410.506',
 'lon_dir': 'E',
 'gps_qual': 1,
 'num_sats': '04',
 'horizontal_dil': '2.6',
 'altitude': 100.0,
 'altitude_units': 'M',
 'geo_sep': '-33.9',
 'geo_sep_units': 'M',
 'age_gps_data': '',
 'ref_station_id': '0000'}
msg.unnamed_values()
Out[10]: []
repr(msg)
Out[11]: "<GGA(timestamp=datetime.time(18, 43, 53, 70000), lat='1929.045', lat_dir='S', lon='02410.506', lon_dir='E', gps_qual=1, num_sats='04', horizontal_dil='2.6', altitude=100.0, altitude_units='M', geo_sep='-33.9', geo_sep_units='M', age_gps_data='', ref_station_id='0000')>"

Some points to consider:

  • Cast known/named values to their actual types e.g. float? This would be a separate issue/PR where probably the parsing of messages would have to be considered.
  • Unit tests

@coveralls
Copy link

coveralls commented Jun 30, 2020 •
edited

Coverage Status

Coverage decreased (-1.0%) to 97.106% when pulling 69d1a9c on ptoews:fields_extraction into 4527cf4 on Knio:master.

@xOneca
Copy link
Contributor

xOneca commented Jul 2, 2020

Could have been used keys() and values() (and items()?), like dicts do.

@xOneca
Copy link
Contributor

xOneca commented Jul 2, 2020

My comment was on named_values() only. Also consider collections.namedtuple's _asdict().

@ptoews
Copy link
Author

ptoews commented Jul 3, 2020

Are you referring to the names of the new methods? I agree there might be better alternatives.
But it's not that simple: There are fixed fields per Sentence class, which are named in fields, but there might also be additional values in data without any name. So a single items() or asdict() would be misleading, since not all the values are named and therefore are not suitable for a dict, and otherwise not all information would be included in the extracted data container.

@xOneca
Copy link
Contributor

xOneca commented Jul 3, 2020

Just give them a name like _extra (or something like that) and let it be the extra data as a list, or add each one as _extra1, etc. Or fieldN.

@ptoews
Copy link
Author

ptoews commented Jul 4, 2020

Thanks for your suggestion, I think this is indeed a better solution.

@ptoews
Copy link
Author

ptoews commented Oct 10, 2020

Unit tests are now done as well. Anything else to do?

@howroyd
Copy link
Contributor

howroyd commented Jan 29, 2024

Unit tests are now done as well. Anything else to do?

Awesome work.

Would be good for the readme to show an example.

@ptoews
Copy link
Author

ptoews commented Apr 28, 2024

Done, let me know if there is anything else

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extracting data container
4 participants
@ptoews @coveralls @xOneca @howroyd