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 --no-magic and --info #159

Closed
wants to merge 0 commits into from
Closed

Add --no-magic and --info #159

wants to merge 0 commits into from

Conversation

sol
Copy link
Owner

@sol sol commented May 4, 2017

No description provided.

@angerman
Copy link

angerman commented May 4, 2017

Hi @sol, sorry for providing such bad quality PRs. Thank you for adding the functionality!

I'm just unsure if a change to the external api warrants a bump to 0.12?

@angerman
Copy link

angerman commented May 4, 2017

I have encoded doctest >= 0.11.3 in the cabal doctest action now. So if this is released as 0.11.3, we should be good.

@angerman
Copy link

What's holding the merge off? Can I help out somehow?

@angerman
Copy link

@sol, can we merge this please?

@sol
Copy link
Owner Author

sol commented May 29, 2017

@angerman Sorry for the late reply!

Regarding your first question, I think we only added new functionality. We did not remove or change existing functionality. This is a backward compatible change and does not require a major version bump. (please correct me if you think I missed something)

From what I said earlier at #156 (comment):

What I now did is: Instead of reviewing your code, I refactored the code and added --no-magic and --info myself (see #159).

Except for the unit tests, my code is untested (that is I did not try it manually).

Can you chime in and

At the point you merged haskell/cabal#4480, I did not see any use of --info. Did I miss something? Is it used by now? Can you point me to the relevant Cabal code?

Can I still ask you for a code review on this PR?

@angerman
Copy link

I'm perfectly fine with the minor version bump.

Ok. just a quick note regarding the --info flag. Cabal queries apps for --version, and the version check is for 0.11.3 or later. I've just noticed that the command description is wrong now (yey strings!)

I'll see to fixing the cabal command description, and give this another review hopefully later tonight.

@angerman
Copy link

--info will likely come in handy later, when cabal doctest (and cabal new-doctest) is being extended. I'm find with the info format as it's the same ghcs settings (and the new llvm-targets file in ghc 8.4) uses.

@sol
Copy link
Owner Author

sol commented May 29, 2017

Cabal queries apps for --version

From what I see at https://github.com/haskell/cabal/pull/4480/files#diff-dce7c7ee4de26e5ec445b5db4e52c0beR319, Cabal does not query apps for --version in general. I assume we could easily change that to use --info?

@sol sol force-pushed the no-magic branch 2 times, most recently from d2b4394 to c76e6e8 Compare June 15, 2017 07:20
@sol sol closed this Jun 15, 2017
@sol sol deleted the no-magic branch June 15, 2017 07:32
@angerman
Copy link

Is this the definitive end of --no-magic for doctest?

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.

2 participants