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

Move previewer task and channel into functions #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danielsoutar
Copy link

@danielsoutar danielsoutar commented Feb 20, 2022

Hi,

Hope this MR makes ReTest a little more accessible to newcomers. This MR is pretty much done, maybe bar one or two points to clarify (specifically what align_overflow and maxidw are for in the ReTest.jl), although I am failing about 7 or so tests. These are:

e: Test Failed at /path/to/ReTest.jl/test/runtests.jl:988
z4: Test Failed at /path/to/ReTest.jl/test/runtests.jl:1007
z4: Test Failed at /path/to/ReTest.jl/test/runtests.jl:1007
has fails: Test Failed at /path/to/ReTest.jl/test/runtests.jl:1251
a2: Test Failed at /path/to/ReTest.jl/test/runtests.jl:1300
b1: Test Failed at /path/to/ReTest.jl/test/runtests.jl:1304
c1: Test Failed at /path/to/ReTest.jl/test/runtests.jl:1308

Some of these are under a header named Main.FailingLoops, so I'm not sure if these are meant to fail or not?

This is not meant to introduce any new functionality - just better structure and documentation for myself and others' benefit when reading the code.

EDIT: All the tests above have failing conditions, so this is fine. As an aside, it might be helpful to separate these into a 'failing tests' file so that it's easier for someone to know that this is expected behavior.

@danielsoutar
Copy link
Author

danielsoutar commented Feb 20, 2022

For those that can write/merge to this repo - I have other code I would like to split out into functions. Let me know if you'd prefer to consolidate those into this branch, or if you'd rather have smaller and more focused MRs instead.

@rfourquet
Copy link
Collaborator

Thanks that looks great! The retest code has indeed grown way too big and I've wanted to see it split into smaller parts for a while. As a side note, I also believe that there might be a better design for handling the different tasks (previewer, printer, worker), but haven't gone around it yet. But this PR is definitely a very good step. Give me a few days to review. And yes, I would definitely "rather have smaller and more focused MRs instead."
I'm super happy to see contributions to this package!

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.

None yet

2 participants