Feature/remove functionality level

Grifs commented 9 months ago

Describe your changes

Remove the functionality layer from the config and move all fields to the top layer.

Related issue(s)

Closes #500

Type of Change

[x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] New functionality (non-breaking change which adds functionality)
[ ] Major change (non-breaking change which modifies existing functionality)
[ ] Minor change (non-breaking change which does not modify existing functionality)
[ ] Bug fix (non-breaking change which fixes an issue)
[ ] This change requires a documentation update

Checklist

Requirements:

[x] I have read the CONTRIBUTING doc.
[x] I have performed a self-review of my code by checking the "Changed Files" tab.
[x] My code follows the code style of this project.

Tests:

[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Documentation:

[x] Proposed changes are described in the CHANGELOG.md.
[x] I have updated the documentation accordingly.

Test Environment

codecov[bot] commented 9 months ago

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (9d9e5a7) 92.51% compared to head (dbb1237) 92.46%.

Files	Patch %	Lines
src/main/scala/io/viash/config/package.scala	93.75%	4 Missing :warning:
src/main/scala/io/viash/ViashTest.scala	80.00%	2 Missing :warning:
src/main/scala/io/viash/config/Config.scala	96.42%	2 Missing :warning:
src/main/scala/io/viash/Main.scala	75.00%	1 Missing :warning:
...c/main/scala/io/viash/runners/NextflowRunner.scala	92.85%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #649 +/- ## =========================================== - Coverage 92.51% 92.46% -0.06% =========================================== Files 129 127 -2 Lines 3889 3889 Branches 635 623 -12 =========================================== - Hits 3598 3596 -2 - Misses 291 293 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Grifs commented 9 months ago

The built script currently still has the meta_functionality_name variable set to refer to the component's name. Should we add a second meta_component_name together with the matching VIASH_META_COMPONENT_NAME name?

In time the meta_functionality_name could be removed if/when so wanted.

Additionally, ns exec has a functionality-name identifier. Should we also duplicate this with component-name?

rcannood commented 9 months ago

Great stuff! Before I review the PR (phew), some questions:

Is the change still backwards compatible for now?
Do we test whether this backwards compatibility still works?
The built script currently still has the meta_functionality_name variable set to refer to the component's name. Should we add a second meta_component_name together with the matching VIASH_META_COMPONENT_NAME name?

Yes! Or maybe simply meta_name and VIASH_META_NAME?
In time the meta_functionality_name could be removed if/when so wanted.

Yes but let's not rush this
Additionally, ns exec has a functionality-name identifier. Should we also duplicate this with component-name?

Or simply name?

Grifs commented 9 months ago

Great stuff! Before I review the PR (phew), some questions:

Is the change still backwards compatible for now?

Yes, (limited) tests so far "just work". Functionality (no pun intended) is provided by https://github.com/viash-io/viash/pull/649/files#diff-08d6fbd855a302a8c8a679e2d4e5612616e059a9a14020da75697415e974c04cR223

Do we test whether this backwards compatibility still works?

Not yet explicitly (but I believe there is still an external script that is fetched and that is currently still in the old format). It's on my TODO list. (shouldn't take too much time :crossed_fingers: )

The built script currently still has the meta_functionality_name variable set to refer to the component's name. Should we add a second meta_component_name together with the matching VIASH_META_COMPONENT_NAME name?

Yes! Or maybe simply meta_name and VIASH_META_NAME?

:+1:

In time the meta_functionality_name could be removed if/when so wanted.

Yes but let's not rush this

:+1:

Additionally, ns exec has a functionality-name identifier. Should we also duplicate this with component-name?

Or simply name?

:+1:

viash-io / viash