Summary descriptions replaced by more detailed ones #2406

RalfKoban · 2017-09-01T14:42:40Z

Hi,

I know there was no issue for that but it would be really nice if the summary descriptions could be updated so that IntelliSense could show more detailed descriptions than "Summary description for ...".

Best regards,
Ralf Koban

CharliePoole

This looks OK to me pending some other team member comments on use of Remarks.

CharliePoole · 2017-09-01T14:58:36Z

src/NUnitFramework/framework/Attributes/SetCultureAttribute.cs

+    /// The culture remains set until the test or fixture completes and is then reset to its original value.
+    /// <para />
+    /// If you wish to use the current culture setting to decide whether to run a test, use the <see cref="CultureAttribute"/> instead of this one.
+    /// </remarks>


Two uncertainties here...

Where do remarks show up? If they don't come up when I right-click, I don't see any advantage to having them - better to expand the summary. We've discussed this before but I can't remember the resolution - I think we decided not to use Remarks. @nunit/framework-team ?

I don't know about putting usage info like "If you wish to use..." in the Intellisense. Seems like it gets in the way, giving the reader something extra to look at each time and figure out if they are in the right place SetCulture seems pretty clear to me.

<remarks> are invisible to IntelliSense unless you are missing the <summary>, in which case it replaces it. Since we require <summary>, the Object Browser is the only place in the IDE that you can see <remarks>, and hypothetically if we generated website content it would show up there.

If the information is interesting to the general user, I'd say it should be in the summary because putting it in <remarks> gives the false impression that people are going to see it and benefit from it. If we agree on this I can add this rationale to our best practices page. I want to know what others think.

I'm actually amenable to this kind of documentation. Not that the user doesn't know what SetCulture means, but just a way to notify you that the other thing exists if that interests you. I could go either way.

If we were to generate website content, then I'd be in favor of putting the first para in the summary and the second in remarks. In fact, that's what I suggest we do here.

As an experienced user, I'm frequently annoyed at Microsoft for telling me "obvious" things, without the ability to turn off the stream of advice. I don't want us to do that.

If the Intellisense has a sentence suggesting I may not be using the correct attribute, I'm going to pause for a few seconds, read it and think about it. I don't want to do that in this case. I think that comparisons with other attributes belong in the docs. Warnings about use of an attribute only belong here in the case where that use is usually wrong. And, of course, if an attribute is usually used incorrectly, we shold probably change it!

I think that comparisons with other attributes belong in the docs.

If we went the direction of generating website content from XML docs, which would it be? Comparison in the docs = website = XML docs, or not?

Yes. I think that would be a good direction.

But please see my comment about what I think should be done with the current PR in the first paragraph of my last comment. Let's resolve this PR rather than getting into general discussions about the website here... that's a separate matter and doesn't actually involve me.

Ah, I mixed up which sections you meant. I don't think that's how <para> is meant to be used, but I could be wrong. I've only ever seen it used to contain each paragraph as contents.

Anyway, I'm good with that. @RalfKoban Does that make sense?

Reading through the comments it doesn't really seem clear whether or not we want to use the <remarks> tag or not? Seems that all the reasoning generally points to no.

@jnm2 Yes, I'm OK with that.

CharliePoole · 2017-09-01T14:59:42Z

src/NUnitFramework/framework/Attributes/SetUICultureAttribute.cs

+    /// <para />
+    /// If you wish to use the current culture setting to decide whether to run a test, use the <see cref="CultureAttribute"/> instead of this one.
+    /// </remarks>
+    /// <seealso cref="SetCultureAttribute"/>


See SetCulture comments

jnm2 · 2017-09-01T16:24:18Z

@RalfKoban Thanks for taking it on yourself to improve this!

The implementation is basically a copy-paste of the ValueTuple implementation with corrections of types and accessing properties instead of fields.

ValueTuple and Tuple now share functionality. For comparers the functionality is placed in the common base-class TupleComparerBase. For MsgUtils the common code can be "specialised" via method arguments to TryFormatTupl and FormatTuple. The tests are kept distinct for clarity.

Add support for tuple

…master

jnm2 · 2017-09-05T14:40:06Z

@RalfKoban Just fyi, rebases are preferable on feature and PR branches rather than merges. The diff gets confusing to review with a normal merge. What I recommend using to update to the latest upstream master is git pull -r upstream master– this fetches upstream/master and then rebases (replays your changes on top of the new tip), giving you a chance to resolve conflicts (if any) for each of your commits.

If that's too arcane for your interests at the moment, don't worry about it and we'll squash your changes into a single commit when we merge the PR. Git can be confusing and it's totally up to you how much time you want to sink into it. =) If you have questions I'm happy to help.

OmicronPersei · 2017-09-05T15:08:01Z

Also, @RalfKoban , I recommend creating a temporary backup branch of your feature branch before you rebase. It helps in the event that you mess up manually merging conflicts... I can speak from experience that it helps 😄 .

RalfKoban · 2017-09-05T15:47:05Z

@jnm2 , @OmicronPersei : Sorry, the merge should not have happened. Did not want to merge it into nunit master branch. Instead I wanted to merge the latest changes into my own nunit branch / fork.

jnm2 · 2017-09-05T16:39:55Z

@RalfKoban Well no, you did what you intended to: you merged the latest changes from upstream/master into your own branch in your own fork. That's just not the best for GitHub PRs though, we can't see the diff properly. That's why I was recommending that you not merge the latest changes from upstream/master into your own branch, but rather rebase your own branch on top of upstream/master. Does that sorta make sense?

[Don't worry, you can't mess up master. It takes two framework team members to merge into master.]

OmicronPersei

Before we merge, we need all of the <remarks> fields to be moved into their respective <summary> sections instead.

OmicronPersei · 2017-09-01T21:20:08Z

src/NUnitFramework/framework/Attributes/SetCultureAttribute.cs

+    /// The culture remains set until the test or fixture completes and is then reset to its original value.
+    /// <para />
+    /// If you wish to use the current culture setting to decide whether to run a test, use the <see cref="CultureAttribute"/> instead of this one.
+    /// </remarks>


Reading through the comments it doesn't really seem clear whether or not we want to use the <remarks> tag or not? Seems that all the reasoning generally points to no.

RalfKoban · 2017-09-08T15:34:33Z

As I'm little bit new to Git/GitHub, I'll close this pull request and will create another one which will contain the wanted changes on the remarks tag.

jnm2 · 2017-09-08T15:37:15Z

@RalfKoban Actually please don't do that! We'd prefer if you reopen and then push your changes to your master branch since that's what's driving the PR. They will show up here automagically. =)

(Also if you're wanting to do a force push, force pushing to a pr branch is expected and welcome.)

Addressed

jnm2

Looks good but one more thing: according to the docs, <para> should not be self-closing but should rather contain each paragraph:

    /// <para>
    /// Sets the current Culture for the duration of a test.
    /// </para>
    /// <para>
    /// It may be specified at the level of a test or a fixture.
    /// ...
    /// </para>

The first <para> there may not be necessary so that is probably up to your personal style, but I do think that <para> should only show up if it contains text.

RalfKoban · 2017-09-08T16:24:44Z

@jnm2 OK, I'll refactor that as well. Just as info - both ways are working; <para /> simply adds a new line.

jnm2 · 2017-09-08T16:31:51Z

@RalfKoban I expect it's similar to how <p/> works in browsers even though it's not idiomatic HTML. (In fact, HTML is exactly what happens to the <para/> were we to drive website or CHM documentation with it.) We should follow the robustness principle.

jnm2

LGTM

Summary descriptions replaced by more detailed ones

aed022c

CharliePoole reviewed Sep 1, 2017

View reviewed changes

mikkelbu and others added 4 commits September 4, 2017 22:58

Add support for Tuple

f259dbb

The implementation is basically a copy-paste of the ValueTuple implementation with corrections of types and accessing properties instead of fields.

Merge pull request #2415 from mikkelbu/add-support-for-tuple

e7c7826

Add support for tuple

Merge branch 'master' of https://github.com/RalfKoban/nunit.git into …

32b729c

…master

OmicronPersei previously requested changes Sep 5, 2017

View reviewed changes

RalfKoban closed this Sep 8, 2017

Summaries reworked

46fef21

RalfKoban reopened this Sep 8, 2017

jnm2 requested changes Sep 8, 2017

View reviewed changes

<para /> changed to <para>...</para>

12499e3

jnm2 approved these changes Sep 8, 2017

View reviewed changes

mikkelbu merged commit d9b2a2d into nunit:master Sep 10, 2017

mikkelbu added closed:done is:docs labels Sep 10, 2017

mikkelbu added this to the 3.9 milestone Sep 10, 2017

Summary descriptions replaced by more detailed ones #2406

Summary descriptions replaced by more detailed ones #2406

Uh oh!

Conversation

RalfKoban commented Sep 1, 2017

Uh oh!

CharliePoole left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CharliePoole Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnm2 commented Sep 1, 2017

Uh oh!

jnm2 commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OmicronPersei commented Sep 5, 2017

Uh oh!

RalfKoban commented Sep 5, 2017

Uh oh!

jnm2 commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OmicronPersei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfKoban commented Sep 8, 2017

Uh oh!

jnm2 commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnm2 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfKoban commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnm2 commented Sep 8, 2017

Uh oh!

jnm2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CharliePoole Sep 1, 2017 •

edited

Loading

jnm2 commented Sep 5, 2017 •

edited

Loading

jnm2 commented Sep 5, 2017 •

edited

Loading

jnm2 commented Sep 8, 2017 •

edited

Loading

jnm2 left a comment •

edited

Loading

RalfKoban commented Sep 8, 2017 •

edited

Loading