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

Add test data files to extra-source-files #1605

Merged
merged 3 commits into from Mar 23, 2021

Conversation

jneira
Copy link
Member

@jneira jneira commented Mar 22, 2021

Comment on lines 31 to 34
-- eval plugin test data is used in the test suite
plugins/hls-eval-plugin/test/testdata/*.yaml
plugins/hls-eval-plugin/test/testdata/*.hs
plugins/hls-eval-plugin/test/testdata/*.hs.expected
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is kinda hacky to me... Let's move eval plugin's tests into its own package, like Sandy did in #1425

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree but it deserves its own pr? This one only makes the hack more evident 😃

Copy link
Member Author

Choose a reason for hiding this comment

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

@berberman we can wait for such pr and rebase, if it is about to be done, of course

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm working on this. I think we need extract hls-test-utils to a standalone package, with some test framework stuffs re-exported, so that tests extracted from HLS could reuse these code.

Copy link
Member Author

@jneira jneira Mar 22, 2021

Choose a reason for hiding this comment

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

I will remove eval test data files and wait for #1606

Copy link
Collaborator

@isovector isovector left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for catching this, and for the easy fix!


library
exposed-modules: Ide.Plugin.Brittany
hs-source-dirs: src
build-depends: base
, brittany >= 0.13.1.0
build-depends: base >=4.12 && <5
Copy link
Member

Choose a reason for hiding this comment

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

Why not base ^>= 4.12?

Copy link
Member Author

@jneira jneira Mar 22, 2021

Choose a reason for hiding this comment

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

I copied it from the main package so I would keep it for consistency, this way we will accept base > 4.12 && < 5, not sure if it is a good thing though.
But I would change it in another pr, if you don't mind.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, the plan sounds good to me

Copy link
Member

@Ailrun Ailrun left a comment

Choose a reason for hiding this comment

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

Many thanks :)

@jneira jneira added the merge me Label to trigger pull request merge label Mar 23, 2021
@mergify mergify bot merged commit 74cfe30 into haskell:master Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
4 participants