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

Allow user to select library or executable #22

Merged
merged 10 commits into from
Oct 22, 2021

Conversation

chiroptical
Copy link
Collaborator

Closes #18

The only real question is do we expect more libraries than executables? I think the answer is yes but maybe you have an opinion

hooks/post_gen_project.py Outdated Show resolved Hide resolved
@utdemir
Copy link
Owner

utdemir commented Oct 6, 2021

Thank you!

The only real question is do we expect more libraries than executables? I think the answer is yes but maybe you have an opinion

I personally use this more often for libraries, however even when I am writing an executable I also have an internal library doing most of the stuff, and a very thin executable section mostly for invoking the library. The main reason is to be able to import the library from the test-suite's.

For the same reason, I have a slight preference for formulating the question more like "do you need an executable section?" rather than "is this a library or an executable?"; because I think it is closer to what we are doing. But if you prefer it this way, I am happy with it too.

I think it would also be good to have another test for this functionality too. Ideally, we'd test every combination of options ((,) <$> [useHpack, useCabal] <*> [Library, Executable]. I think it still is doable when we only have a few toggles, but it might get unwieldy. So, feel free to add either a few manual test cases, or a slightly more complex logic testing all combinations of the project_configuration_tool and project_type parameters.

@chiroptical
Copy link
Collaborator Author

chiroptical commented Oct 6, 2021

For the same reason, I have a slight preference for formulating the question more like "do you need an executable section?" rather than "is this a library or an executable?"; because I think it is closer to what we are doing. But if you prefer it this way, I am happy with it too.

Yeah, I like the former as well. Another option, maybe, would be to add a commented executable section to *.cabal, package.yaml just so it is easy to include one if you want. I agree that most of the time I am opening up the library section first.

@utdemir
Copy link
Owner

utdemir commented Oct 9, 2021

Another option, maybe, would be to add a commented executable section to *.cabal, package.yaml just so it is easy to include one if you want.

But then, would we still include the main module (and possibly a separate directory)? That doesn't sound ideal to me, I think your approach on this PR is better.

If you change the prompt to something along the lines of "include executable section/stanza/module" (feel free to pick a better wording) instead of "library or executable", and add some test cases, I'm happy with this PR merged :).

If you don't have time, I'd be happy to work on this too, just let me know :).

Copy link
Owner

@utdemir utdemir 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 to me, just a few cosmetic comments :).

.github/workflows/build.yaml Outdated Show resolved Hide resolved
.github/workflows/build.yaml Outdated Show resolved Hide resolved
ci.nix Outdated Show resolved Hide resolved
ci.nix Outdated Show resolved Hide resolved
cookiecutter.json Outdated Show resolved Hide resolved
hooks/post_gen_project.py Outdated Show resolved Hide resolved
.github/workflows/build.yaml Outdated Show resolved Hide resolved
@chiroptical chiroptical force-pushed the add-executable-section-as-option branch from 20a9677 to 0bcc466 Compare October 21, 2021 12:26
@chiroptical
Copy link
Collaborator Author

The multiline string thing doesn't appear to work. I have no clue how to get it to pass CI. I think I'll revert that for now and submit an issue

@chiroptical
Copy link
Collaborator Author

Updated! I think this will be a nice ergo boost for users!

@utdemir
Copy link
Owner

utdemir commented Oct 22, 2021

This is great, thanks!

@utdemir utdemir merged commit 9247811 into utdemir:master Oct 22, 2021
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.

Add the 'executable' section conditionally
2 participants