Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Close #2661 - add no-compile flag #2709

Closed
wants to merge 7 commits into from

Conversation

robertmagier
Copy link
Contributor

I throw an error when flags --compile-all or --debug are used together with -compile-none
Not sure if this is the correct way to do it.


    if (config.compileAll && config.compileNone) {
      options.logger.log(
        "--compile-none can not be set together with --compile-all or --debug"
      );
      throw "Command line error.";
    }

@robertmagier robertmagier changed the title Close #2661 Close #2661 - add no-compile flag Dec 26, 2019
Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

Thanks for this! Just a minor cleanup issue I spotted.

packages/core/lib/test.js Outdated Show resolved Hide resolved
@robertmagier robertmagier requested a review from gnidan December 28, 2019 17:15
@robertmagier
Copy link
Contributor Author

@gnidan Should I add test case for --compile-none flag scenario ?

@robertmagier
Copy link
Contributor Author

robertmagier commented Dec 30, 2019

I added test file to test how to use --compile-none flag. In last scenario this test is failing exactly as in issue #469 Can someone verify if this scenario is correct ? @gnidan ?

@robertmagier
Copy link
Contributor Author

@gnidan @CruzMolina Pleeeease ;)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 72.95% when pulling f06da62 on robertmagier:develop into 7c19713 on trufflesuite:develop.

@gnidan
Copy link
Contributor

gnidan commented Aug 7, 2020

Ended up taking a different approach, but this should be done now as of #3208.

Sorry for the radio silence here, but I think this PR can now be closed, since that functionality now exists!

@gnidan gnidan closed this Aug 7, 2020
@robertmagier
Copy link
Contributor Author

All my work for nothing.

@gnidan
Copy link
Contributor

gnidan commented Aug 28, 2020

I'm really sorry about that @robertmagier! I felt awful but still decided that the alternate approach was simple enough.

Looking at this again now, however, I think your tests will provide lots of value! I'll reopen this with the plan to get these tests merged, at least!

@gnidan gnidan reopened this Aug 28, 2020
@eggplantzzz eggplantzzz self-assigned this Sep 1, 2020
@eggplantzzz
Copy link
Contributor

eggplantzzz commented Sep 2, 2020

Some of this work was taken and used in #3335. I gave coauthoring credit to @robertmagier. Apologies @robertmagier for letting this one become irrelevant :(

@robertmagier
Copy link
Contributor Author

I am sorry for whining ;) Thank you for giving coauthoring credit to me as well. Let me know if you need any help here... or there.

@eggplantzzz
Copy link
Contributor

It's no problem :) I hate to have good work go to waste, it has been a busy time the last year or so but we have implemented some process to help us to stay on top of contributions like this one. Is your Windows CI PR still relevant?

@eggplantzzz
Copy link
Contributor

So this feature was merged in this PR and should be released next week!

@eggplantzzz
Copy link
Contributor

Closing as this feature was released in version 5.1.47. Thanks!

@eggplantzzz eggplantzzz closed this Oct 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants