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

Tidy up decoder 2 #392

Merged
merged 13 commits into from
Feb 17, 2021
Merged

Tidy up decoder 2 #392

merged 13 commits into from
Feb 17, 2021

Conversation

Urhengulas
Copy link
Member

This PR does actually quite some restructuring of the defmt_decoder. The main ones are:

  • make fn parse_* and fn get_locations methods of struct Table
  • move struct Frame and struct Decoder into own private module each to make code easier digestable
  • various simplifications

open questions

  • First question: What do you thing of splitting it up like this?
  • Second question: Should fn decode maybe also become a method of Table?

The tests can likely also get split up more into each submodule, but I'd do this in another PR.

@Urhengulas Urhengulas self-assigned this Feb 16, 2021
Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@jonas-schievink
Copy link
Contributor

Second question: Should fn decode maybe also become a method of Table?

Yeah, this definitely makes sense to me

Urhengulas and others added 2 commits February 17, 2021 19:59
Co-authored-by: Jonas Schievink <jonas.schievink@ferrous-systems.com>
@Urhengulas
Copy link
Member Author

I've added decode to Table and included your suggestion. Are we good to go?

@jonas-schievink
Copy link
Contributor

Yeah, looks good!

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 17, 2021

Build succeeded:

@bors bors bot merged commit 476b524 into main Feb 17, 2021
@bors bors bot deleted the tidy-up-decoder branch February 17, 2021 19:22
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

Successfully merging this pull request may close these issues.

2 participants