Skip to content

Pass on files and excludeFiles from config to registry #967

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

Closed

Conversation

i-am-the-slime
Copy link
Contributor

@i-am-the-slime i-am-the-slime commented Jun 25, 2023

Add files to config and enable colocation by adding excludeFiles, too

Adds files and excludeFiles to the publish config.
Those are then passed to the registry.

This is required for purescript/registry-dev#602

Checklist:

  • Added the change to the "Unreleased" section of the changelog
  • Added some example of the new feature to the README
  • Added a test for the contribution (if applicable)

Comment on lines +292 to +297
# optional, globs of additional files to publish
files:
- "src-extra/**/*.purs"
# optional, globs of files to exclude from publishing
excludeFiles:
- "src/**/*Test.purs"
Copy link
Member

Choose a reason for hiding this comment

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

This PR might not be aiming at implementing the full colocation feature, but I'd like to see it through to get a feel at how complex it is.

Once we forgo the idea that all sources are in src and all the test files are in test, we need more config than this:

  1. files will add source files to the ones in src
  2. excludeFiles will remove source files from the ones in src
  3. then we'll need a config key to add test files to the ones in test
  4. ..and something to remove test files from the ones in test I guess?

We'd want (3) and (4) for symmetry if we have (1) and (2), but I question the utility of (1) and (4), as they are not necessary for colocation and I can't come up with non-contrived usecases for having them.

Then something else that we'd need to care about (and that it not yet implemented here) is how we deal with the globs for src and test in the presence of colocation.
This is a good starting point, but there might be more occurrences across the codebase.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that we could rename the keys to includes and excludes, and do two passes. First, we add anything from includes to the src we take by default. Then we apply excludes to that set (disallowing you from excluding anything you can't exclude, like your spago.yaml). Since we don't publish the test directory anyway I'm not sure if it's necessary that it has special handling.

Copy link
Member

@f-f f-f Jul 3, 2023

Choose a reason for hiding this comment

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

Colocation is not only about publishing though, there's building and testing involved in there too. Modifying the config format happens in core, and we'd need to think the whole feature through when doing so, which includes thinking about how to include files for spago test to pick up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your input @f-f .
I indeed had not considered testing in this change.

For my personal needs, I need a way to exclude certain files from the libraries I publish.
In particular, these are unit tests, end-to-end-tests any kinds of tests in between. Then there are Story.purs files for storybook that I'd like to colocate but not ship.

I personally don't have a need for excluding what to build. I would want to always compile all .purs files in my project including unit tests, stories, and end-to-end tests.

I have done this in the past similarly to this:
image

The problem which I noticed later came when running storybook or spec on an application that dependend on this library it would include the specs and stories from the lib. Not nice.

So I also can't find a use case for 4 in this:

but I question the utility of (1) and (4), as they are not necessary for colocation and I can't come up with non-contrived usecases for having them

Once we forgo the idea that all sources are in src and all the test files are in test

Also here, I'm afraid to question everything at this stage (and I'm probably lacking some background), but is there really this dichotomy of sources and tests? Or does test stand for anything that shouldn't ship?

I personally have quick and slow (end-to-end) tests that I invoke differently (via npm currently), and I have these storybook files which (in my head at least) are more like visual tests without any real assertions.

So actually, I'm not sure from the current README how you'd translate a project with various spago.dhall files (spago.test.dhall, spago.e2e.dhall, spago.stories.dhall) to the new spago.yaml format.
Would you do this with a small top-level workspace and then subfolders for each of those?
In that case I could imagine a use-case for the files key:

d2

Okay, I'll stop writing here now, maybe it makes sense to discuss this via voice one day so I don't run too far in the wrong direction, and let me know if all this is just draining you and is too annoying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@f-f What change are you requesting?

Copy link
Member

@f-f f-f Jul 13, 2023

Choose a reason for hiding this comment

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

For my personal needs, I need a way to exclude certain files from the libraries I publish.
In particular, these are unit tests, end-to-end-tests any kinds of tests in between. Then there are Story.purs files for storybook that I'd like to colocate but not ship.

I understand your specific usecase, but we can't really afford to just implement a feature that follows a usecase verbatim, or we'd quickly end up with unmaintainable feature bloat - the issue with that is that the interplay between features often incurs quadratic maintenance complexity. Because of this it is worth to take a step back and try to make the feature more generic than what we need right now, so that it could cover more usecases.

In this case, one related usecase that came up at work is that one of the Erlang build tools we use treats the test folder in a certain way that doesn't work with FFI, so we'd need to either make the test folder configurable, or add a way to add sources for spago test to pick up. Since Colocation is also very much about "changing what is picked up by spago test" we could cover both cases with this same feature.

Also here, I'm afraid to question everything at this stage (and I'm probably lacking some background), but is there really this dichotomy of sources and tests? Or does test stand for anything that shouldn't ship?

It's not a dichotomy as much as a convention. As of today:

  • all the sources for a package are in src
  • all the test files for a package are in test
  • both of these folders get built when you build a package, but spago run only includes sources, while spago test will include both
  • all of the src folder - and only that - ends up published

The whole premise of this new feature is that this convention needs to go, so things need to be configurable instead.
With colocation our definition of "source files" would change from "src" to "src minus whatever we exclude", and our definition of "test files" would change from "src + test" to "src minus whatever we exclude, plus test, plus whatever additional test sources"

What change are you requesting?

Given the above reasoning, I think we could get away with:

  • configuration changes:
    • add a package.build.exclude key: a list of globs of files to exclude from the src directory, when building and when publishing
    • add a package.test.include key: a list of globs of files to include when running test (note: these are in addition to the above, and the test directory)
    • add a package.publish.include key: a list of globs of files to add when publishing (these don't have to be source files), to pass it through to the registry
  • propagate all of these new globs to the whole building process, starting from here
  • there might be various ancillary changes, e.g. one I can think about right now is:
    • when you add a package to the package set, we try to read its spago.yaml - we should detect the package.build.exclude key and use the new glob instead of blindly building the whole of src

Would you do this with a small top-level workspace and then subfolders for each of those?

Yes, I would unpack the end-to-end test to be in its own package, that can be spago run when you need it (rather than running every time you need to run tests)

Copy link
Member

Choose a reason for hiding this comment

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

Also, I'm up for talking this through if that helps to unpack things!

Though we should then report back here, so there's a trace of what was discussed and what the reasoning was, for when we want to refer to these in the future

Copy link
Member

Choose a reason for hiding this comment

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

We’d need a package.publish.exclude, since this issue in particular is more about removing files from src when publishing than it is about including them. So we’d end up with package.publish.include + package.publish.exclude.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, and then we won't need the package.build.exclude

@i-am-the-slime
Copy link
Contributor Author

i-am-the-slime commented Jul 13, 2023

TODO:

  • package.publish.exclude
  • package.test.include
  • package.publish.include (replaces files)

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.

3 participants