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 'Introduced in Catch 2...' to docs, for recent Catch releases #1695

Merged
merged 15 commits into from
Aug 2, 2019

Conversation

claremacrae
Copy link
Contributor

Description

This is a work-in-progress - more changes coming later

Add a mechanism - scripts/extractFeaturesFromReleaseNotes.py to generate text for pasting in to docs/*.md, to indicate the release number in which recent features were added.

GitHub Issues

Partial implementation of this part of #1667

For recently-added features, add "Introduced in Catch 2.x.y" to the documentation - see https://docs.gitlab.com/ee/ci/variables/#variable-types for an example

Notes

So far, I've just added the text 'Introduced in Catch 2.9.0.' - to get feedback on whether this is the kind of thing you expected, @horenmar ...

After initial feedback, I'll fill in the text for more releases.

@claremacrae
Copy link
Contributor Author

I’ll have a look at the failing builds - I wasn’t expecting documentation-only changes to break the builds, so didn’t run tests...

@horenmar
Copy link
Member

That's due to 52f3aba, as it turns out, path normalization is tricky.

@claremacrae
Copy link
Contributor Author

Thanks

So do you have any comments on the changes I made, or am I OK to carry on with some recent releases?

@horenmar
Copy link
Member

Yeah, this looks perfectly fine. There is an open question though, if there is both an issue and a PR related to the feature, which one do you link?

@claremacrae
Copy link
Contributor Author

Yeah, this looks perfectly fine. There is an open question though, if there is both an issue and a PR related to the feature, which one do you link?

Having looked at a few of these pairs, I recommend linking the PR...

  • I think the PR tends to have more useful, more concise info
  • The PR more obviously refers back to the issue near its start
  • Whereas issues can have lots of discussion, with the PR tending to get linked at the end
  • Hence the PR is a more useful starting point
  • The PR is more likely to be closer to the final implementation,
  • Sometimes the author of the issue proposes an implementation that may differ from the final solution, such as a different command-line arg. This could confuse users if we linked to the issue.

So I would select the PR as the link to include in the docs.

I found that github links to PRs work even if you put /issues/ in the URL - e.g. #1614 redirects to #1614 - so I didn't make the script detect whether a link was a PR or an issue... Partly for simplicity, and partly because it looked at first glance like more links in the release notes were issues than PRs.

@horenmar
Copy link
Member

Okay, that sounds good.

@codecov
Copy link

codecov bot commented Jul 21, 2019

Codecov Report

Merging #1695 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1695   +/-   ##
=======================================
  Coverage   83.91%   83.91%           
=======================================
  Files         123      123           
  Lines        3405     3405           
=======================================
  Hits         2857     2857           
  Misses        548      548

@codecov
Copy link

codecov bot commented Jul 21, 2019

Codecov Report

Merging #1695 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #1695      +/-   ##
=========================================
+ Coverage   86.18%   86.2%   +0.02%     
=========================================
  Files         136     136              
  Lines        5180    5174       -6     
=========================================
- Hits         4464    4460       -4     
+ Misses        716     714       -2

@claremacrae
Copy link
Contributor Author

Release 2.7.0 made a number of small changes to the generator documentation:
v2.6.1...v2.7.0#diff-34031d7827c96c080087fe785df095fd

These are summarised here:
https://github.com/catchorg/Catch2/releases/tag/v2.7.0

I've been unable to see succinct ways to insert 'Introduced in Catch 2.7.0.' notes for these changes.

Suggestions appreciated, if you think it's worth doing,,,

@claremacrae
Copy link
Contributor Author

My current thinking on this PR is to go as far back as v2.3.0 which was released on 23 Jul 2018, i.e. a year ago...

That'll take a few more hours, so I might stop sooner - but I'm thinking I'll keep going as I have been learning useful things about Catch2 along the way...

@@ -96,6 +96,8 @@ scope and thus capturing references is dangerous. If you need to use
variables inside the generator expression, make sure you thought through
the lifetime implications and use `GENERATE_COPY` or `GENERATE_REF`.**

**Note**: `GENERATE_COPY` and `GENERATE_REF` were introduced in Catch 2.7.1.
Copy link
Member

Choose a reason for hiding this comment

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

I think this would look better if it used the same convention as the rest of these...

GENERATE_COPY and GENERATE_REF were introduced in Catch 2.7.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! Done in 3d91219 ...

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 later realised that you meant to use the Quote style - with > prefix on this line, which is much better,

I've now done that for all in 3f04c2f.

@claremacrae
Copy link
Contributor Author

Hi @horenmar

I believe that I have now updated the docs for all documented features from 2.3.0 to 2.9.1 inclusive...

Other than reviewing the later changes, I think outstanding questions are:

  1. Are you happy that this is far enough back?
  2. How can we ensure that corresponding comments get added routinely as part of future releases? Adding them after-the-fact turned out to be very time-consuming...

One idea would be:

  • have a convention to use when adding to new documentation, such as > Introduced in Catch [not yet released].
  • And then build in to your release process a step of inserting the correct version number

A nice by-product of that will be to prevent users accidentally trying to use features that have not yet been released.

Whatever the conclusion, it would be good to have instructions added to the contributing page...

@claremacrae
Copy link
Contributor Author

@horenmar
Copy link
Member

  1. Going a year back should be plenty enough.
  2. I can script up a replacement of specific marker with the newly generated version easily enough, the important thing is to pick one, and to ensure that the markers will be there when the time comes to release the new version.

I think the best way is to standardize on a small set of markers, IIRC right now only two kinds are used, the simple

> [Introduced](issue-link) in Catch X.

and, when more specificity is needed:

> X (Y and Z) was [introduced](issue-link) in Catch X.

So we could say that when the version is not yet known, it should be written down as > [Introduced](issue-link) in Catch X.Y.Z and the release* scripts would then replace this placeholder with the newly released version.

We will also need to document this information in docs/contributing.md.

@claremacrae
Copy link
Contributor Author

That sounds great - thank you.

@claremacrae
Copy link
Contributor Author

@horenmar It looks like my edits aren't yet merged. If that's true, is there anything else that you would like done?

I'm only asking as I'd like to avoid anyone making conflicting edits in the files I changed...

@horenmar
Copy link
Member

@claremacrae I'd like to have the changes to contributing.md done as part of this PR before merging it.

@claremacrae
Copy link
Contributor Author

@claremacrae I'd like to have the changes to contributing.md done as part of this PR before merging it.

OK thanks.

Did you want to do that, or would you like me to have a go?

@horenmar
Copy link
Member

I'd prefer if you did it.

@claremacrae
Copy link
Contributor Author

Ah right, OK sure, happy to do that.

horenmar added a commit that referenced this pull request Aug 2, 2019
@horenmar
Copy link
Member

horenmar commented Aug 2, 2019

@claremacrae I threw together some documentation, can you take a look?

@claremacrae
Copy link
Contributor Author

@horenmar It's great - thank you very much!

@claremacrae
Copy link
Contributor Author

Oh - @horenmar - maybe add a note to add this at the bottom of new pages too:

---

[Home](Readme.md#top)

horenmar added a commit that referenced this pull request Aug 2, 2019
@horenmar
Copy link
Member

horenmar commented Aug 2, 2019

Okay, I'll rebase the PR in the evening and then merge it.

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.

2 participants