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

Hotfix for #2051 #2064

Merged
merged 8 commits into from
Dec 23, 2020
Merged

Hotfix for #2051 #2064

merged 8 commits into from
Dec 23, 2020

Conversation

drug007
Copy link
Contributor

@drug007 drug007 commented Dec 22, 2020

Hot fix #2051

@dlang-bot
Copy link
Collaborator

Thanks for your pull request and interest in making D better, @drug007! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

@drug007
Copy link
Contributor Author

drug007 commented Dec 22, 2020

The PR makes dub test command run unit tests in the main source file in case of single file package (by default dub excludes them). The second test was added to make sure that unit tests run in fact.

@thewilsonator
Copy link
Contributor

thewilsonator commented Dec 22, 2020

It seems you removed an _ from the test files name, creating a large diff. Please put it back to make reviewing easier.

@drug007
Copy link
Contributor Author

drug007 commented Dec 22, 2020

Ok, I'll do renaming in another PR then (to conform to other file names).

before GeneratorSettings ignored this option
because it represents all package by definition
else dub doesn't find it because the single file package do not have the default package directory structure
no string processing to evaluate the result
to check if unit tests from the main source file run at all. Because by default the main source file is excluded from tests.
@drug007
Copy link
Contributor Author

drug007 commented Dec 22, 2020

Rebased the branch

@dlang-bot dlang-bot merged commit cfd23b4 into dlang:master Dec 23, 2020
@drug007
Copy link
Contributor Author

drug007 commented Dec 23, 2020

@thewilsonator between what is preferable - many small specific commits like I did or one large commit like it was told in other PRs I saw?

@drug007 drug007 deleted the issue2051-final-fix branch December 23, 2020 07:39
@thewilsonator
Copy link
Contributor

Specifically in this case, file renames in separate PRs is preferable.

@drug007
Copy link
Contributor Author

drug007 commented Dec 23, 2020

I've realized that GitHub is not able to handle renaming gracefully and shows both files separately that makes review very difficult. My question is more about is splitting a PR to many small commits better than one single large commit? For me it's much easier to review step by step by means of small, clean and dedicated commits than one large commit with all changes at once but I saw in other PRs the advice to squash all commits so I'm curious what approach is better here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running unit tests from DUB single file packages fails
3 participants