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

V03 wip issue575 ... adding "flow" component, and flowMain option. #588

Merged
merged 10 commits into from
Oct 31, 2022

Conversation

petersilva
Copy link
Contributor

so this doesn't do the whole job of the issue, but I think it is useful on it's own, and it lets @andreleblanc11 's stuff work (he's using it in his branch also.)

it adds:
-- the ability to declare flow/ component configurations that are vanilla.
-- the ability to declare flowMain to run arbitrary subclasses of flow making it possible to define all existing components as config files.
-- include files to implement all the existing components.
-- English and French documentation of the above.

…ut containing second

implementation of post, shovel, watch, and winnow components.
Allows andre to use a stock component to host his amserver plugin.
allows all existing components to be built from pure configuration
files.  kind of next step in #575
@petersilva
Copy link
Contributor Author

I didn't merge this because I thought some discussion made sense... it is based on feedback from people about not liking or being confused initially by the concept of "components" ... so when I looked at what #379 that plugin from @andreleblanc11 cannot work with any existing component, so needed vanilla... question is... should we make everything vanilla, so there are no components any more? that would be future work... this just makes "vanilla" (aka: flow/ ) work

Right now, flow/ is just added as an additional component... but it results in having two of everything... one implementation of each component as a sub-class, and another as a config file. Some of them are redundant.

Copy link
Member

@reidsunderland reidsunderland left a comment

Choose a reason for hiding this comment

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

I like the changes. The explanations make sense.

To avoid the redundancy, for shovel, winnow, post and watch, we could just use flowMain shovel, etc. right? Since we have to keep some subclasses, it doesn't seem terrible to keep the others even if they're just a storage location for the default config options

@petersilva
Copy link
Contributor Author

I didn't think adding three reviewers meant all three have to approve... oh well. we can discuss on Monday, and if bless it then. would be nice to have the additional reviews, but not the end of the world.

Copy link
Contributor

@MagikEh MagikEh left a comment

Choose a reason for hiding this comment

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

Looks good, love the docs changes

Copy link
Contributor

@tysonkaufmann tysonkaufmann left a comment

Choose a reason for hiding this comment

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

Looks good

@petersilva petersilva merged commit c18a041 into v03_wip Oct 31, 2022
@petersilva petersilva deleted the v03_wip_issue575 branch May 13, 2023 17:29
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.

4 participants