Feature/ted 175 #46

kaleanych · 2022-04-01T08:03:47Z

No description provided.

…ed-175

codecov · 2022-04-01T08:07:42Z

Codecov Report

Merging #46 (48b8a45) into main (78a6092) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
+ Coverage   98.60%   98.71%   +0.10%     
==========================================
  Files          42       43       +1     
  Lines        1576     1630      +54     
==========================================
+ Hits         1554     1609      +55     
+ Misses         22       21       -1

Impacted Files	Coverage Δ
.../data_manager/adapters/mapping_suite_repository.py	`99.31% <100.00%> (+<0.01%)`	⬆️
ted_sws/notice_transformer/adapters/rml_mapper.py	`100.00% <100.00%> (ø)`
...ormer/entrypoints/cmd_mapping_suite_transformer.py	`100.00% <100.00%> (ø)`
...rmaliser/entrypoints/generate_mapping_resources.py	`100.00% <0.00%> (+5.88%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78a6092...48b8a45. Read the comment docs.

kaleanych · 2022-04-01T08:10:47Z

SETUP:

pip install https://github.com/meaningfy-ws/ted-sws/archive/main.zip
Usage:
# transformer --help

costezki

Well done!

Please see the comments below.

costezki · 2022-04-05T19:23:24Z

.coveragerc

@@ -0,0 +1,3 @@
+[report]
+exclude_lines =


interesting, what else could be added to such a file and what would be useful to us?

These lines are not interpreted by by python when running coverage.
Will help to not be frustrated by (<100%) coverage for command line tools

costezki · 2022-04-05T19:24:52Z

dags/notice_transformer_dag.py

+sys.path = list(set(sys.path))
+import os
+
+os.chdir("/opt/airflow/")


is this "hack" permanent? Or can we have something done at the "system" level?

As I understood from Stefan, this is a requirement for DAG to run smooth in airflow environment

costezki · 2022-04-05T19:26:00Z

setup.py

@@ -70,6 +70,7 @@ def open_local(paths, mode="r", encoding="utf8"):
    entry_points={
        "console_scripts": [
            # "rdfpipe = rdflib.tools.rdfpipe:main", # inspired form rdflib, replace as needed
+            "transformer = ted_sws.notice_transformer.entrypoints.cmd_mapping_suite_transformer:main"


costezki · 2022-04-05T19:29:58Z

ted_sws/notice_transformer/entrypoints/cmd_mapping_suite_transformer.py

+import os
+import sys
+
+module_path = os.path.dirname(os.path.realpath(__file__)) + '/../../../'


This looks very dangerous and easy to break.
One shall rather use pkg_resources following the example from metadata_normaliser.

try: import importlib.resources as pkg_resources except ImportError: # Try backported to PY<37 `importlib_resources`. import importlib_resources as pkg_resources

This was added to be run without python package manager (to get used faster by semantic team). It will be updated, so it runs as a package command

costezki · 2022-04-05T19:31:28Z

ted_sws/notice_transformer/entrypoints/cmd_mapping_suite_transformer.py

+   # make install && make local-dotenv-file 
+   (.env file must already be created)
+4) Go to ted-sws-artefacts directory.
+5) Create a symlink in ted-sws-artefacts project's root directory:


Certainly, the symlink is not the right way to do this.
Rather, this shall be facilitated through pip install of the ted-sws repository configured through setup.py

Already answered above: this was implemented to get faster to semantic team (It will be updated so it can be used as Python package)

costezki · 2022-04-05T19:32:12Z

ted_sws/notice_transformer/entrypoints/cmd_mapping_suite_transformer.py

+              help='MappingSuite ID to be processed (leave empty to process all Mapping Suites).')
+@click.option('--opt-mappings-path', default=DEFAULT_MAPPINGS_PATH)
+@click.option('--opt-output-path', default=DEFAULT_OUTPUT_PATH)
+def transform_notice(mapping_suite_id, opt_mapping_suite_id, opt_mappings_path, opt_output_path):


fair enough!

costezki · 2022-04-05T19:34:41Z

tests/unit/notice_transformer/conftest.py

+
+@pytest.fixture
+def cmd_transformer_path() -> str:
+    return "../../../../ted_sws/notice_transformer/entrypoints/cmd_mapping_suite_transformer.py"


This approach is suboptimal, and using a relative path like that is not safe at all.
It is much more elegant, easy and safe if one employs the testing facilities offered by the Click library. See the testing section in the documentation.

costezki · 2022-04-05T19:35:19Z

tests/unit/notice_transformer/test_cmd_mapping_suite_transformer.py

+
+
+def test_cmd_transformer(fake_mapping_suite_id, fake_repository_path):
+    response = cmdRunner.invoke(transform_notice, [fake_mapping_suite_id, "--opt-mappings-path", fake_repository_path])


I see you found the clirunner in the Click documentation :))

costezki · 2022-04-05T19:35:46Z

tests/unit/notice_transformer/test_cmd_mapping_suite_transformer.py

+    __process_output_dir(fake_repository_path, fake_mapping_suite_id)
+
+
+def __test_cmd_transformer_from_cli(fake_repository_path, cmd_transformer_path, fake_mapping_suite_id):


why is this test commented out?

removed it. it was for locally testing the command as it runs as a cli script

already fixed

Kolea Plesco and others added 13 commits March 28, 2022 15:28

added DAG

5b3ad9d

Update transformer_dag.py

6378252

added CMD

3e5a5dc

Merge branch 'main' of github.com:meaningfy-ws/ted-sws into feature/t…

91deab1

…ed-175

Finished base Transformer CMD

efc92ff

Finished base Transformer CMD

8c9dff0

Finished base Transformer CMD + Updated test

333b8f4

Updated test coverage

93627e1

Updated test coverage

a0cb0a5

Updated test coverage

3e37512

Updated test coverage

654ec45

Updated test coverage

e4ea2ab

Updated test coverage

99ddf31

kaleanych requested review from costezki, Dragos0000 and CaptainOfHacks April 1, 2022 08:03

Removed the DAG trigger

14f97e7

Kolea Plesco added 3 commits April 1, 2022 11:18

Test refactoring

155ec85

Test refactoring

f56d095

Added transformer script to setup.py

19ed97f

kaleanych requested review from costezki and removed request for Dragos0000 and costezki April 1, 2022 12:36

costezki previously requested changes Apr 5, 2022

View reviewed changes

Kolea Plesco and others added 4 commits April 6, 2022 13:15

Updated ted-sws package setup

8dd76d6

Merge conflicts updates

51d949d

Merge conflicts updates

4b04070

Merge branch 'main' into feature/ted-175

c52624c

Kolea Plesco added 3 commits April 6, 2022 13:37

merged with main

98ab26f

merged with main

985315b

merged with main / fixed conflicts

48b8a45

kaleanych requested review from costezki and removed request for CaptainOfHacks April 6, 2022 11:00

costezki approved these changes Apr 6, 2022

View reviewed changes

kaleanych merged commit 106a270 into main Apr 6, 2022

costezki deleted the feature/ted-175 branch April 7, 2022 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/ted 175 #46

Feature/ted 175 #46

kaleanych commented Apr 1, 2022

codecov bot commented Apr 1, 2022 •

edited

Loading

kaleanych commented Apr 1, 2022 •

edited

Loading

costezki left a comment

costezki Apr 5, 2022

kaleanych Apr 6, 2022

costezki Apr 5, 2022

kaleanych Apr 6, 2022

costezki Apr 5, 2022

costezki Apr 5, 2022

kaleanych Apr 6, 2022

costezki Apr 5, 2022

kaleanych Apr 6, 2022

costezki Apr 5, 2022

costezki Apr 5, 2022

costezki Apr 5, 2022

costezki Apr 5, 2022

kaleanych Apr 6, 2022



		def test_cmd_transformer(fake_mapping_suite_id, fake_repository_path):
		response = cmdRunner.invoke(transform_notice, [fake_mapping_suite_id, "--opt-mappings-path", fake_repository_path])

		__process_output_dir(fake_repository_path, fake_mapping_suite_id)


		def __test_cmd_transformer_from_cli(fake_repository_path, cmd_transformer_path, fake_mapping_suite_id):

		@@ -0,0 +1,3 @@
		[report]
		exclude_lines =

Feature/ted 175 #46

Feature/ted 175 #46

Conversation

kaleanych commented Apr 1, 2022

codecov bot commented Apr 1, 2022 • edited Loading

Codecov Report

kaleanych commented Apr 1, 2022 • edited Loading

costezki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 1, 2022 •

edited

Loading

kaleanych commented Apr 1, 2022 •

edited

Loading