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

Possibility to dump tree as json #1

Merged
merged 6 commits into from
Feb 6, 2024
Merged

Conversation

mieczkowski
Copy link

Hello,

I need possibility to dump tree to list of records (to store in DB for example), so I've added two methods:

  • MarshalJSON to marshal tree (separated ipv4 and ipv6 keys) to json.

  • DumpList to build custom json struct, use for example in database storage, or for debug purpose.

Test data is taken from stringify_test.go.

@coveralls
Copy link

coveralls commented Feb 5, 2024

Pull Request Test Coverage Report for Build 7800491566

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • -2 of 36 (94.44%) changed or added relevant lines in 1 file are covered.
  • 12 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.6%) to 94.894%

Changes Missing Coverage Covered Lines Changed/Added Lines %
jsonify.go 34 36 94.44%
Files with Coverage Reduction New Missed Lines %
stringify.go 12 85.59%
Totals Coverage Status
Change from base Build 7790117356: 0.6%
Covered Lines: 762
Relevant Lines: 803

💛 - Coveralls

@gaissmai
Copy link
Owner

gaissmai commented Feb 5, 2024

@mieczkowski thanks for contribution. Since you didn't open an issue before, I updated the main branch already by accident. Please rebase your PR on it.
I'm not happy with the public DumpList, please make it private and please check again the docstrings, I've seen at least one typo.
Best regards
Charly

... and by reading the code (well done!) I've just some minor remarks:

  • rename dumpList to dumpListRec
  • rename DumpList to dumpList
  • please change the signatures without error results, there is no error generating code in these fuctions

just my five cents

@mieczkowski
Copy link
Author

Good catch with errors handling - I've simplified code. However I can not test one error check, because there is no chance to simulate this error, except some I/O errors inside json.Marshal:

buf, err := json.Marshal(result)
if err != nil {
	return nil, err
}

Please consider leaving DumpList as public function. In my case I have list of subnets in DB (flat list). I need to rebuild tree after every operation on this with some validations, and:

  1. Marshaling into json is helpful for rest api (returning sorted tree to frontend)
  2. DumpList is helpful for add/remove operations and update DB

Without this method I need to marshal json and parse it into map[string] . Unnecessary waste of resources and without it I will end up with fork - and I don't want that ;)

Copy link
Owner

@gaissmai gaissmai left a comment

Choose a reason for hiding this comment

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

please see my inline comment to DumpList

jsonify.go Outdated
@@ -44,40 +37,28 @@ func (t *Table[V]) MarshalJSON() ([]byte, error) {
}

// DumpList dumps ipv4 od ipv6 tree into list of roots and their children
Copy link
Owner

Choose a reason for hiding this comment

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

still a typo in this public docstring and a missing dot at end of line. If this method has to be public, please invest more time to find a clear description or spend an extra test or maybe a dumplist_example_test.

Copy link
Author

Choose a reason for hiding this comment

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

I've fixed both docstrings.

I don't think that we need separated tests for DumpList. It will be duplicate of json tests.

Copy link
Owner

Choose a reason for hiding this comment

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

The test for DumpList was not meant for testing, just to better explain the public method, but I can live without or add it myself later.

Two more small remarks:

  • public type ListElement needs a docstring.
  • please rename field Cidr to CIDR, for golang naming convention, like HTTP and not Http
    Regards
    Charly

Copy link
Author

Choose a reason for hiding this comment

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

Done

@gaissmai
Copy link
Owner

gaissmai commented Feb 6, 2024

What do you think about the following naming:

type DumpListElement[V any] struct {}
func (t *Table[V]) DumpList(is4 bool) []DumpListElement[V]

background: the type only deals with dumping and marshaling, nothing with the real bart-Table algo.
or maybe: DumpListNode, since it's a recursive datastructure

@mieczkowski
Copy link
Author

What do you think about the following naming:

type DumpListElement[V any] struct {}
func (t *Table[V]) DumpList(is4 bool) []DumpListElement[V]

background: the type only deals with dumping and marshaling, nothing with the real bart-Table algo. or maybe: DumpListNode, since it's a recursive datastructure

I vote for DumpListNode , looks better than element since it's a tree :)

@gaissmai gaissmai merged commit 69cef36 into gaissmai:main Feb 6, 2024
6 checks passed
@gaissmai
Copy link
Owner

gaissmai commented Feb 6, 2024

merged, thanks for contribution

@mieczkowski mieczkowski deleted the json_dump branch February 6, 2024 13:40
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.

3 participants