Fix Archive::describe to include sub-archives (#188) #189

InfinityByTen · 2020-05-29T12:07:41Z

Added to template adding describe to optional resources.
Prepended a newline for readability.

Signed-off-by: Aseem Dua <[email protected]>

VeaaC · 2020-06-02T05:48:36Z

Can you add some unit tests as well (GeneratedArchiveTests)?:

test that a failed sub-archive can be described
test that a failed optional sub-archive can be described

Test that if sub-archive is unintialized, it's in the describe. Test if schema mismatches in optional sub-archive, you get a fatal error and it is in the describe. The mandatory sub-archive case is present in an existing test. Signed-off-by: Aseem Dua <[email protected]>

VeaaC · 2020-06-07T08:38:53Z

flatdata-cpp/test/GeneratedArchiveTest.cpp

 }

+TEMPLATE_TEST_CASE_METHOD( Fixture,
+                           "Uninitialized sub-archive is descried",


desried -> described

Ah sure, will fix.

VeaaC · 2020-06-07T08:43:34Z

flatdata-cpp/include/flatdata/Archive.h

+    std::string description;
+    if ( initialized )
+    {
+        description = std::is_base_of< Archive, ResourceType >::value


What about:

describing resourcing is sub-archives that failed to initialize? How does that look like now?

describing resources in sub-archives that successfully loaded, how does that look like now?

I guess it might need to look like a tree-structure, e.g.:

resource1 : ... archive_resource1: ... sub_resource1: ... sub_resource2: ...

Uninitialized is reported as an error only in mandatory case. But then it doesn't stop us from describing the Archive child resources instead of keeping it "N/A". Will adapt it.

If everything is initialized, then then describe works but there is no indentation and everything is flattened out (it's flatdata after all). But I'll try to see if there's a nice way to get indentation.

Would you suggest a tree structure with complete descriptions like so? :

Looks a bit hacky... how about having a single combined list, but full resource names:

outermost outer outer.outer1 outer.outer2 outer.inner outer.inner.inner

That could require passing down the path-prefix, and being able to disable headers/footers during recursive calls

Well, first versions are mostly hacky. Anyhow, I've reworked it a bit based on your inputs. I would like to retain the tree structure and somehow include which ones are archives and which are plain structs. Here's how the latest version looks:

Let me know what you think.

Looks good!

Signed-off-by: Aseem Dua <[email protected]>

Whether a resource is initialized or not, it will always display some details. For optional entities, a dummy is created to extract the name of the type to give maximum information to the user. Nested levels do not print headers. Signed-off-by: Aseem Dua <[email protected]>

InfinityByTen · 2020-07-10T09:56:02Z

I seem to often run into this issue of tests failing on CI. This time it happened because I just had forgotten where to run the tests from. Should we have a separate task to put into a README how to run tests locally?

Signed-off-by: Aseem Dua <[email protected]>

VeaaC · 2020-07-10T11:26:02Z

I seem to often run into this issue of tests failing on CI. This time it happened because I just had forgotten where to run the tests from. Should we have a separate task to put into a README how to run tests locally?

That makes sense: Some How to contribute section with advice how to test the individual components:

How to use local generator via export FLATDATA_GENERATOR_PATH=${PWD}/../flatdata-generator
python -m nose for generator tests
how to manually run generator for a test to get a new reference, e.g. ./generator.py -s ../test_cases/archives/multivector.flatdata -g rust -O tests/generators/rust_expectations/archives/multivector.rs.1
how to update the checked in generated rust test code, e.g.: ../flatdata-generator/generator.py -s lib/src/test/test.flatdata -g rust -O lib/src/test/test_generated.rs && sed -i 's/flatdata::/crate::/g' ../flatdata-rs/lib/src/test/test_generated.rs
etc

Do you want to create a new issue for that? And would you like to fill that yourself, or have it filled by regular contributors?

Signed-off-by: Aseem Dua <[email protected]>

InfinityByTen · 2020-07-10T16:28:19Z

Do you want to create a new issue for that? And would you like to fill that yourself, or have it filled by regular contributors?

I can create an issue as a starting point, definitely. But I guess I don't have enough insight as to what all it could include. Like I literally had no idea about half the things you mentioned.

VeaaC · 2020-08-04T10:19:40Z

flatdata-cpp/include/flatdata/Archive.h

+    }
+    else
+    {
+        auto dummy = new ResourceType( );


You are leaking memory here, just create the object on the stack?

VeaaC · 2020-08-04T10:19:52Z

flatdata-cpp/include/flatdata/Archive.h

+    const bool is_archive = std::is_base_of< Archive, ResourceType >::value;
+
+    const ResourceType ref = initialized ? *resource                  // valid ref
+                                         : *( new ResourceType( ) );  // ref to dummy, not used


You are leaking memory here, just create the object on the stack?

VeaaC · 2020-08-04T10:20:47Z

flatdata-cpp/include/flatdata/ArrayView.h

    size_t size_in_bytes( ) const;
    size_t size( ) const;
-    std::string describe( ) const;
+    std::string describe( size_t unused = 0u ) const;


give unused arguments their normal name, so users know what they are

VeaaC · 2020-08-04T10:21:12Z

flatdata-cpp/include/flatdata/MemoryDescriptor.h


    std::string
-    describe( ) const
+    describe( size_t unused = 0 ) const


size_t /name_of_variable/ ?

VeaaC · 2020-08-04T10:24:53Z

flatdata-cpp/test/GeneratedArchiveTest.cpp

+Resource                             Optional  Too Large  Loaded    Details
+================================================================================
+outer                                NO        NO         YES       Structure of size 1
+archive_resource                     YES       NO         NO        FATAL: Resource storage not initialized. Please check archive path.


I guess the FATAL should be removed, since it is not fatal for optional subarchives

Same if true for check archive path.

Can you rephrase it into a neutral message? Or have a different message for root and nested?

Signed-off-by: Aseem Dua <[email protected]>

InfinityByTen force-pushed the fix_describe_sa branch from 6a7f4ba to f3c3392 Compare May 29, 2020 12:39

Fix Archive::describe to include sub-archives (heremaps#188)

62f2e19

Signed-off-by: Aseem Dua <[email protected]>

InfinityByTen force-pushed the fix_describe_sa branch from f3c3392 to c700fd1 Compare May 31, 2020 07:54

Fix tests

a4346ab

Signed-off-by: Aseem Dua <[email protected]>

InfinityByTen force-pushed the fix_describe_sa branch from c700fd1 to a4346ab Compare May 31, 2020 07:56

Fix cpp_expectations

93bb151

Signed-off-by: Aseem Dua <[email protected]>

InfinityByTen force-pushed the fix_describe_sa branch from dbc939f to 4a6369c Compare June 5, 2020 16:53

VeaaC reviewed Jun 7, 2020

View reviewed changes

InfinityByTen added 2 commits July 10, 2020 07:47

Minor: Fix the README

e3aaa4f

Signed-off-by: Aseem Dua <[email protected]>

Fix generator tests.

59bd294

Signed-off-by: Aseem Dua <[email protected]>

Typo fix

73fb369

Signed-off-by: Aseem Dua <[email protected]>

InfinityByTen mentioned this pull request Jul 10, 2020

Create a How to contribute section #192

Open

VeaaC reviewed Aug 4, 2020

View reviewed changes

VeaaC approved these changes Aug 7, 2020

View reviewed changes

Remove fatal errors from node description and fix memory leaks

6ea5e28

Signed-off-by: Aseem Dua <[email protected]>

InfinityByTen force-pushed the fix_describe_sa branch from 50a8f62 to 6ea5e28 Compare August 7, 2020 10:24

Merge branch 'master' into fix_describe_sa

f27c938

VeaaC merged commit 8839fb3 into heremaps:master Aug 7, 2020

Fix Archive::describe to include sub-archives (#188) #189

Fix Archive::describe to include sub-archives (#188) #189

Uh oh!

Conversation

InfinityByTen commented May 29, 2020

Uh oh!

VeaaC commented Jun 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

InfinityByTen commented Jul 10, 2020

Uh oh!

VeaaC commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

InfinityByTen commented Jul 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

VeaaC commented Jul 10, 2020 •

edited

Loading