Issue 2052 wikipedia link from street relation #2622

strump · 2022-05-26T07:09:09Z

strump commented

2022-05-26 07:09:09 +00:00

Closes #2052

Added special case when associatedStreet relation has wikipedia tag then this tag should not be copied to relation members.

Closes #2052 Added special case when `associatedStreet` relation has `wikipedia` tag then this tag should not be copied to relation members.

strump reviewed 2022-05-26 09:07:22 +00:00

generator/generator_tests/relation_tags_tests.cpp

strump commented

2022-05-26 07:12:45 +00:00

relation_tag.cpp code doesn't override ref tag of route members.
But in relation_tag.cpp line 93 ref tag is added. Looks like it's ignored.

I expect that road11 ref tag contains SP62;IT:RA/SP60 but it doesn't.

`relation_tag.cpp` code doesn't override `ref` tag of route members. But in [`relation_tag.cpp` line 93](https://github.com/organicmaps/organicmaps/blob/master/generator/relation_tags.cpp#L93=) ref tag is added. Looks like it's ignored. I expect that road11 `ref` tag contains `SP62;IT:RA/SP60` but it doesn't.

biodranik (Migrated from github.com) requested changes 2022-05-26 09:27:12 +00:00

biodranik (Migrated from github.com) left a comment

Thanks!

generator/generator_tests/relation_tags_tests.cpp

biodranik (Migrated from github.com) commented

2022-05-26 09:20:35 +00:00

What is the meaning of UNREACHABLE here? It it should not be called, please use CHECK(false, ("Should not be called"));

What is the meaning of UNREACHABLE here? It it should not be called, please use `CHECK(false, ("Should not be called"));`

biodranik (Migrated from github.com) commented

2022-05-26 09:21:53 +00:00

nit: it can be refactored to return std::optional instead of bool and element-by-reference.

biodranik (Migrated from github.com) commented

2022-05-26 09:22:13 +00:00

One line please )

biodranik (Migrated from github.com) commented

2022-05-26 09:22:52 +00:00

It fits into one line.

biodranik (Migrated from github.com) commented

2022-05-26 09:23:15 +00:00

One line.

biodranik (Migrated from github.com) commented

2022-05-26 09:23:18 +00:00

One line.

biodranik (Migrated from github.com) commented

2022-05-26 09:23:29 +00:00

One line.

biodranik (Migrated from github.com) commented

2022-05-26 09:23:42 +00:00

One line.

biodranik (Migrated from github.com) commented

2022-05-26 09:24:49 +00:00

One line. Or, if its too long, prefer the following syntax:

{
{},
{},
};

It allows inserting new values without modifying the first and last lines.

One line. Or, if its too long, prefer the following syntax: ``` { {}, {}, }; ``` It allows inserting new values without modifying the first and last lines.

biodranik (Migrated from github.com) commented

2022-05-26 09:25:00 +00:00

One line.

biodranik (Migrated from github.com) commented

2022-05-26 09:25:05 +00:00

One line.

generator/generator_tests/relation_tags_tests.cpp

					
				@ -0,0 +7,4 @@

				// TODO: Rewrite these tests using RelationTagsEnricher with some test mock of IntermediateDataReaderInterface.

				namespace relation_tags_tests

				{

				using namespace feature;

biodranik (Migrated from github.com) commented

2022-05-26 09:19:31 +00:00

Please sort.

generator/relation_tags.cpp

					
				@ -100,7 +100,8 @@ void RelationTagsWay::Process(RelationElement const & e)

				    return;

				  bool const isBoundary = (type == "boundary") && IsAcceptBoundary(e);

biodranik (Migrated from github.com) commented

2022-05-26 09:26:42 +00:00

  bool const isAssociatedStreet = type == "associatedStreet";
  bool const processAssociatedStreet = isAssociatedStreet &&
                                       Base::IsKeyTagExists("addr:housenumber") &&
                                       !Base::IsKeyTagExists("addr:street");

to avoid unnecessary string comparison.

```suggestion bool const isAssociatedStreet = type == "associatedStreet"; bool const processAssociatedStreet = isAssociatedStreet && Base::IsKeyTagExists("addr:housenumber") && !Base::IsKeyTagExists("addr:street"); ``` to avoid unnecessary string comparison.

biodranik (Migrated from github.com) reviewed 2022-05-26 09:27:46 +00:00

generator/generator_tests/relation_tags_tests.cpp

biodranik (Migrated from github.com) commented

2022-05-26 09:27:46 +00:00

@vng please take a look

strump reviewed 2022-05-26 09:52:20 +00:00

generator/generator_tests/relation_tags_tests.cpp

					
				@ -0,0 +7,4 @@

				// TODO: Rewrite these tests using RelationTagsEnricher with some test mock of IntermediateDataReaderInterface.

				namespace relation_tags_tests

				{

				using namespace feature;

strump commented

2022-05-26 09:52:20 +00:00

Reordered.

strump reviewed 2022-05-26 09:53:32 +00:00

generator/generator_tests/relation_tags_tests.cpp

strump commented

2022-05-26 09:53:31 +00:00

Replaced with CHECK(false, ...).
Also replaced in collector_building_parts_tests.cpp.

Replaced with `CHECK(false, ...)`. Also replaced in `collector_building_parts_tests.cpp`.

strump reviewed 2022-05-26 09:55:02 +00:00

generator/generator_tests/relation_tags_tests.cpp

strump commented

2022-05-26 09:55:01 +00:00

Switch to optional wold be great. But it requires a lot of changes in intermediate_data.hpp, intermediate_data.cpp, etc. Not sure I am confident enough.

Switch to `optional` wold be great. But it requires a lot of changes in `intermediate_data.hpp`, `intermediate_data.cpp`, etc. Not sure I am confident enough.

strump reviewed 2022-05-26 09:55:14 +00:00

generator/generator_tests/relation_tags_tests.cpp

strump commented

2022-05-26 09:55:14 +00:00

Reformatted.

strump reviewed 2022-05-26 09:55:27 +00:00

generator/generator_tests/relation_tags_tests.cpp

strump commented

2022-05-26 09:55:27 +00:00

Reformatted to single line

strump reviewed 2022-05-26 09:55:36 +00:00

generator/generator_tests/relation_tags_tests.cpp

strump commented

2022-05-26 09:55:36 +00:00

Reformatted to single line

strump reviewed 2022-05-26 09:55:48 +00:00

generator/generator_tests/relation_tags_tests.cpp

strump commented

2022-05-26 09:55:48 +00:00

Reformatted to single line

strump reviewed 2022-05-26 09:55:56 +00:00

generator/generator_tests/relation_tags_tests.cpp

strump commented

2022-05-26 09:55:56 +00:00

Reformatted to single line

strump reviewed 2022-05-26 09:56:07 +00:00

generator/generator_tests/relation_tags_tests.cpp

strump commented

2022-05-26 09:56:07 +00:00

Reformatted to single line.

strump reviewed 2022-05-26 09:56:17 +00:00

generator/generator_tests/relation_tags_tests.cpp

strump commented

2022-05-26 09:56:17 +00:00

Reformatter to single line.

strump reviewed 2022-05-26 09:56:26 +00:00

generator/generator_tests/relation_tags_tests.cpp

strump commented

2022-05-26 09:56:26 +00:00

Reformatter to single line.

strump reviewed 2022-05-26 09:56:35 +00:00

generator/generator_tests/relation_tags_tests.cpp

strump commented

2022-05-26 09:56:34 +00:00

Reformatted to single line.

strump reviewed 2022-05-26 09:56:43 +00:00

generator/relation_tags.cpp

					
				@ -100,7 +100,8 @@ void RelationTagsWay::Process(RelationElement const & e)

				    return;

				  bool const isBoundary = (type == "boundary") && IsAcceptBoundary(e);

strump commented

2022-05-26 09:56:43 +00:00

Fixed.

vng (Migrated from github.com) reviewed 2022-05-26 10:26:17 +00:00

vng (Migrated from github.com) left a comment

ToDo for @vng:
Current generator tests fixture looks ugly.
Should use RelationTagsEnricher with some test mock of IntermediateDataReaderInterface.

ToDo for @vng: Current generator tests fixture looks ugly. Should use RelationTagsEnricher with some test mock of IntermediateDataReaderInterface.

generator/generator_tests/relation_tags_tests.cpp

vng (Migrated from github.com) commented

2022-05-26 10:24:38 +00:00

Put todo here.

vng commented

2022-05-26 10:27:42 +00:00

(Migrated from github.com)

Please, don't use KR braces style in C++ code:
bad:

UNIT_TEST() {
}

good:

UNIT_TEST()
{
}

Please, don't use KR braces style in C++ code: bad: ``` UNIT_TEST() { } ``` good: ``` UNIT_TEST() { } ```

strump commented

2022-05-26 10:31:24 +00:00

@vng, what text should I add?

TEST_EQUAL(road11.GetTag("ref"), "SP62", ()); // TODO: Use RelationTagsEnricher with some test mock of IntermediateDataReaderInterface.

@vng, what text should I add? ```cpp TEST_EQUAL(road11.GetTag("ref"), "SP62", ()); // TODO: Use RelationTagsEnricher with some test mock of IntermediateDataReaderInterface. ```

vng commented

2022-05-26 10:34:54 +00:00

(Migrated from github.com)

ToDo:

check refs inheritance (expected "IT:RA/SP60;SP62")
rewrite this tests using RelationTagsEnricher with some test mock of IntermediateDataReaderInterface

ToDo: - check refs inheritance (expected "IT:RA/SP60;SP62") - rewrite this tests using RelationTagsEnricher with some test mock of IntermediateDataReaderInterface

vng (Migrated from github.com) approved these changes 2022-05-27 08:47:21 +00:00

vng commented

2022-05-27 08:48:16 +00:00

(Migrated from github.com)

@biodranik If you approve, I'd make squash-and-merge.

biodranik (Migrated from github.com) approved these changes 2022-05-27 10:51:00 +00:00

generator/generator_tests/relation_tags_tests.cpp

biodranik (Migrated from github.com) commented

2022-05-27 10:47:52 +00:00

// TODO: Rewrite these tests using RelationTagsEnricher with some test mock of IntermediateDataReaderInterface.

```suggestion // TODO: Rewrite these tests using RelationTagsEnricher with some test mock of IntermediateDataReaderInterface. ```

biodranik (Migrated from github.com) commented

2022-05-27 10:48:27 +00:00

Выше есть using std; значит надо либо тут убрать, либо убрать using.

biodranik (Migrated from github.com) commented

2022-05-27 10:50:18 +00:00

Есть же using namespace generator. Лучше using namespace generator::cache; и тут укоротить запись. И дальше по коду аналогично.

biodranik (Migrated from github.com) commented

2022-05-27 10:50:49 +00:00

Или убрать вообще using и везде явно прописывать namespace.

strump reviewed 2022-05-30 14:20:11 +00:00

generator/generator_tests/relation_tags_tests.cpp

strump commented

2022-05-30 14:20:11 +00:00

Убрал using namespace std

Убрал `using namespace std`

strump reviewed 2022-05-30 14:22:34 +00:00

generator/generator_tests/relation_tags_tests.cpp

strump commented

2022-05-30 14:22:33 +00:00

Из namespace generic берётся класс RelationTagsWay, который тестируется, и класс generator::cache::OSMElementCacheReaderInterface. Я оставил using namespace generator, но убрал generator:: из кода.

Из namespace generic берётся класс `RelationTagsWay`, который тестируется, и класс `generator::cache::OSMElementCacheReaderInterface`. Я оставил `using namespace generator`, но убрал `generator::` из кода.