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

Integrate docfx #1075

Merged
merged 25 commits into from
Jan 4, 2017
Merged

Integrate docfx #1075

merged 25 commits into from
Jan 4, 2017

Conversation

nirinchev
Copy link
Member

@nirinchev nirinchev commented Dec 28, 2016

TODO:

  • Add some intro to api/index.md.
  • Logo update.
  • Decide if we should use PCL/Xamarin project

Consider namespace changes

  • Realms.ObjectSchema -> Realms.Schema.ObjectSchema
  • Realms.XXXAttribute -> Realms.Attributes.XXXAttribute
  • Realms.XXXException -> Realms.Exceptions.XXXException

Decide what to filter out

  • IRealmObjectHelper
  • RealmObject.{Get|Set}XXXValue
  • WovenAttribute, WovenPropertyAttribute
  • PreserveAttribute

Functional changes

  • Removed RealmObjectAlreadyManagedByRealmException.
  • Made RealmSchema.Builder private.

Resolves #1074
Resolves #854

@kristiandupont
Copy link
Contributor

Is docfx_project a standard name? Seems a bit inconsistent with what we otherwise have..

@nirinchev
Copy link
Member Author

That's the standard folder you get from running docfx init -q, but I guess I could rename it, I don't think any of the functionality is tied to that name.

@nirinchev
Copy link
Member Author

nirinchev commented Dec 28, 2016

Should we exclude obsolete members from the api reference? Ping @AndyDentFree @fealebenpae @kristiandupont

@AndyDentFree
Copy link
Contributor

Should we exclude obsolete members from the api reference?

Good question. I think we keep them.

Whilst code using older NuGets will still compile, they need to be documented as being obsolete. We can't erase outdated code samples floating around the net so it is important that anyone doing a search for those methods will see docs showing their obsolete status and the replacements.

@kristiandupont
Copy link
Contributor

The view source links point to the PCL version of the code. It should probably point to the shared version instead.

@AndyDentFree
Copy link
Contributor

I'd like to remove the Improve this Doc links. We may need to add hooks to notify us of people posting such improvements and I don't think the team is big enough to manage such an open contribution approach.

@AndyDentFree
Copy link
Contributor

Frankly, I don't see the appeal. It has better typography (which can be configured) but loses a number of points of functionality:

  • poorer navigation
  • no easy overview
  • more noise
  • no search.

Copy link
Contributor

@kristiandupont kristiandupont left a comment

Choose a reason for hiding this comment

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

Tiny nits, overall this looks good!

Instructions on how to build from source are included in that repository's `README.md`.


Minimal Sample
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this should be a link to walkthrough docs instead? Seems redundant to maintain another minimal sample with no real added value..

@@ -0,0 +1,14 @@
{{!Copyright (c) Microsoft. All rights reserved. Licensed under the MIT license. See LICENSE file in the project root for full license information.}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this should say Realm Inc instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the original footer which is licensed by MS, I'm not a lawyer, but I don't think we can copyright it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

The change is in the footer text - I replace Microsoft with Realm :)

LINQ support in Realm Xamarin
=============================

To make a query with Realm, you use the `Realm.All<T>()` method to get a `IQueryable<T>` instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these should be cref's, but that is probably outside the scope of this PR.

/// </summary>
public class ObjectSchema : IReadOnlyCollection<Property>
{
/// <summary>
/// Gets the name of the original class declaration from which the schema was built.
/// </summary>
/// <value>The name of the class.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these necessary? Does lacking them make the docs look incomplete or something? They seem really redundant to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

screen shot 2016-12-29 at 12 46 42 pm

vs

screen shot 2016-12-29 at 12 47 03 pm

@@ -139,12 +142,12 @@ public static class CollectionNotificationsExtensions
}

/// <summary>
/// A convenience method that casts <c>IQueryable{T}</c> to <see cref="IRealmCollection{T}"/> which implements INotifyCollectionChanged.
/// <b>Deprecated</b> A convenience method that casts <see cref="IQueryable{T}"/> to <see cref="IRealmCollection{T}"/> which implements INotifyCollectionChanged.
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't docfx respect the [Obsolete] attribute?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't display it very prominently.

/// };
/// </code>
/// </example>
/// <value>Typically left null so by default all <see cref="RealmObject"/>s will be able to be stored in all Realms.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems inconsistent with what the <value> elements normally contain -- this is more like a remark.

@@ -19,7 +19,8 @@
namespace Realms
{
/// <summary>
/// An exception, thrown when trying to write data to the Realm and you haven't begun a Write transaction or when the realm is opened as read-only.
/// An exception, thrown when trying to write data to the <see cref="Realm"/> and you haven't begun a Write
/// <see cref="Transaction"/> or when the <see cref="Realm"/> is opened as read-only.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the exception that is thrown when trying to create a transaction on a readonly realm? Because if not, I think the case is covered by the comment above. Also, it could be reworded to "when trying to write data the realm outside a transaction" or similar.

public HttpStatusCode StatusCode { get; }

/// <summary>
/// Gets the ReasonPhrase of the response.
/// </summary>
/// <value>The ReasonPhrase of the response.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

This, as well as the summary, provides very little information about what this actually is. Maybe it could say "HTTP ReasonPhrase" or something?

@@ -28,11 +28,13 @@ public class SessionErrorException : Exception
/// <summary>
/// Gets the kind of session error this exception represents.
/// </summary>
/// <value>An enum value, describing the root cause of the error.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

The "root cause" could be that the sysadmin who was supposed to keep the server running was lazy. I think this merely describes the error :-)

@nirinchev
Copy link
Member Author

In terms of appeal, here are a few points:

  • It's the next iteration of Sandcastle and as such, looks quite similar to sandcastle generated docs (e.g. Newtonsoft.Json).
  • Most new .NET projects coming from MS use it to drive docs, e.g. Orleans.
  • It has search (no idea why that hasn't rendered, investigating).
  • It has a link to the source code so people can easily check what exactly is happening.
  • It allows plugging in markdown files to supplement the xml docs.
  • While it has its own set of issues, it correctly recognizes c# 6.0 syntax.
  • It has links to the BCL classes (INotifyCollectionChanged, etc.).
  • It shows extension methods in the declaring class - User.GetManagementRealm, Realm.GetSession, etc.
  • It looks more "modern".

@nirinchev
Copy link
Member Author

I deployed the docs on github pages so you can play with it: https://nirinchev.github.io/DocfxTest/index.html

Copy link
Member

@fealebenpae fealebenpae left a comment

Choose a reason for hiding this comment

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

I like the proposed namespace changes.

Why make RealmSchema.Builder private, though? I know we don't expose the API needed to open a Realm with a wholly dynamic schema, but when we do users'll need it to describe the schema programmatically.

@nirinchev
Copy link
Member Author

@fealebenpae I'd say let's keep them private for now (otherwise it's a bit confusing to have them in our public API, but not offer any chance to use them). Once we expose the dynamic opening API, we'll make them public again. How does that sound?

@nirinchev
Copy link
Member Author

The reason why "View Source" links to the pcl is that docfx doesn't play nicely with Xamarin projects - it doesn't resolve BCL references. It works fine with the windows project, but we don't have a windows.sync project to generate the sync docs from. So as far as I can see, we have 4 options here:

  • Generate docs from PCL and have "View Source" point to the dummy implementation for the time being.
  • Generate docs from Xamarin.iOS/Android and have unresolved BCL references (e.g. INotifyCollectionChanged, IList<T>, etc.). This one is not very appealing, because when we have a BCL reference in the XML docs, it will replace it with an empty string (i.e. not only does it not link to msdn, it also will not show the name of the class, which is quite confusing).
  • Create a dummy Win32.Sync project just for docs.
  • Have @fealebenpae ship a working Win32.Sync implementation over the weekend.

Personally, I find Option 4 most appealing, but would be happy to hear your thoughts as well. @kristiandupont @AndyDentFree

@nirinchev
Copy link
Member Author

I decided against the attribute namespace change - mainly because I imagine it will be very common scenario to add attributes to your models, which will make it really annoying to have to add both namespaces.

@nirinchev nirinchev merged commit 4546f72 into master Jan 4, 2017
@nirinchev nirinchev deleted the ni/docfx branch January 4, 2017 23:38
@nirinchev nirinchev removed the S:Review label Jan 4, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
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