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.

Sign up for GitHub

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

Jump to bottom

rdmd_test: simplify flag for specifying default compiler #340

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

rdmd_test: simplify flag for specifying default compiler #340

wants to merge 1 commit into from

Conversation

WebDrake
Copy link
Contributor

@WebDrake WebDrake commented Mar 22, 2018 •
edited

Since it seems there are some use-cases where it is unavoidable to run rdmd_test directly, it's nice if we can reduce the amount of typing required to do so.

Besides the rdmd executable itself, the only other obligatory argument to the test suite is the default compiler expected to be used by rdmd. This patch shortens the long option to --default-compiler and allows an equivalent single-character -D option, so that usage can now be of the form:

rdmd_test -D <default-compiler> <path-to-rdmd-executable>

The posix.mak and win32.mak makefiles have been updated accordingly.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @WebDrake! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@WebDrake
rdmd_test: simplify flag for specifying default compiler
e7ba852 
Since it seems there are some use-cases where it is unavoidable to run
`rdmd_test` directly, it's nice if we can reduce the amount of typing
required to do so.

Besides the rdmd executable itself, the only other obligatory argument
to the test suite is the default compiler expected to be used by rdmd.
This patch shortens the long option to `--default-compiler` and allows
an equivalent single-character `-D` option, so that usage can now be of
the form:

    rdmd_test -D <default-compiler> <path-to-rdmd-executable>

The posix.mak and win32.mak makefiles have been updated accordingly.
stefan-koch-sociomantic
stefan-koch-sociomantic reviewed Mar 22, 2018
View reviewed changes
rdmd_test.d
@@ -55,7 +55,7 @@ int main(string[] args)
string testCompilerList; // e.g. "ldmd2,gdmd" (comma-separated list of compiler names)

auto helpInfo = getopt(args,
"rdmd-default-compiler", "[REQUIRED] default D compiler used by rdmd executable", &defaultCompiler,
"D|default-compiler", "[REQUIRED] default D compiler used by rdmd executable", &defaultCompiler,
Copy link

Choose a reason for hiding this comment

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

Seems to the only capital option ... maybe consider choosing a lower-case letter ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to take suggestions. Although it did seem rather appropriate that it be D ;-)

@WebDrake
Copy link
Contributor Author

Sorry about earlier test failure: I forgot to tweak win32.mak ;-) Fixed now.

@WebDrake WebDrake changed the title Simplify rdmd_test flag for specifying default compiler rdmd_test: simplify flag for specifying default compiler Mar 22, 2018
@WebDrake
Copy link
Contributor Author

This time the test failure looks unrelated to the code. Is there some way to auto-trigger a fresh test from the PR? (I forget... :-\ )

@wilzbach
Copy link
Member

git commit --amend or being a member of the dlang GH organization. With the latter your can directly restart the builds at Travis. I don't know why they don't allow it for the other (maybe to avoid over-using the quota of an organization?) - it's a bit silly because you could do this manually too.
Anyhow, I restarted the failing build for you.

@WebDrake
Copy link
Contributor Author

Some CI setups have hooks that allow PR authors (or maintainers) to trigger fresh builds via a comment "Test this please" or similar. Could be convenient?

Anyway, thanks :-)

@wilzbach
Copy link
Member

Yes, we plan to do this, e.g. dlang/dlang-bot#109 or dlang/dlang-bot#190, but there's only so much one can do in one "day".
(especially if some part of it is taken up by low-impact discussions ;-)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefan-koch-sociomantic stefan-koch-sociomantic stefan-koch-sociomantic left review comments

@andralex andralex Awaiting requested review from andralex andralex is a code owner

@CyberShadow CyberShadow Awaiting requested review from CyberShadow CyberShadow is a code owner

@wilzbach wilzbach Awaiting requested review from wilzbach wilzbach is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
stalled
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@WebDrake @dlang-bot @wilzbach @stefan-koch-sociomantic