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

feat:doc: stub doc headers for database tables #2803

Merged
merged 3 commits into from
Sep 24, 2024
Merged

Conversation

firestack
Copy link
Member

@firestack firestack commented Sep 20, 2024

mix docs doesn't generate anything for these modules if @moduledoc false is set, but we're using TypedEctoSchema now which does generate type information, which can be surfaced by ExDoc.

It's also impossible to link to modules in documentation if @moduledoc false is set.

I've also created a ExDoc group which is enabled via setting the option on @moduledoc section: :ecto, this should make it easier to find our database related code, and easier to navigate in the documentation.


It is possible to do this automatically for Ecto schemas, by setting this attribute in the shared Skate.Schema module. I do think this is neat, but I'm not really certain(i.e., let's discuss) this is a good decision?

diff --git a/lib/skate/schema.ex b/lib/skate/schema.ex
index d18c8b83e3..cde6e814cd 100644
--- a/lib/skate/schema.ex
+++ b/lib/skate/schema.ex
@@ -5,6 +5,8 @@
 
   defmacro __using__(_opts) do
     quote do
+      @module_doc section: :ecto
+
       use TypedEctoSchema
     end
   end

I like that this means we don't need to remember to add section: :ecto to all new Ecto schemas, because we have a growing list of "conventions" which are difficult to keep all in your head. But on the other hand, putting the @module_doc in the schema macro removes the choice later to easily change the section: :ecto value, I looked into making the @module_doc setting conditional, but wasn't very successful on my first attempt.

Copy link

Coverage of commit 4475743

Summary coverage rate:
  lines......: 92.5% (3302 of 3568 lines)
  functions..: 71.8% (1365 of 1901 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/notifications/db/block_waiver.ex                            |66.7%      3|71.4%     7|    -      0
  lib/notifications/db/bridge_movement.ex                         |66.7%      3|71.4%     7|    -      0
  lib/notifications/db/notification.ex                            |66.7%      6|77.8%     9|    -      0
  lib/notifications/db/notification_user.ex                       | 100%      1|80.0%     5|    -      0
  lib/skate/detours/db/detour.ex                                  | 100%      2| 100%     6|    -      0
  lib/skate/settings/db/route_tab.ex                              |66.7%      3|85.7%     7|    -      0
  lib/skate/settings/db/test_group.ex                             | 100%      3|85.7%     7|    -      0
  lib/skate/settings/db/test_group_user.ex                        |66.7%      3|71.4%     7|    -      0
  lib/skate/settings/db/user.ex                                   |66.7%      3|85.7%     7|    -      0
  lib/skate/settings/db/user_settings.ex                          |66.7%      3|85.7%     7|    -      0

Download coverage report

@firestack firestack marked this pull request as ready for review September 20, 2024 16:21
@firestack firestack requested a review from a team as a code owner September 20, 2024 16:21
@firestack firestack force-pushed the kf/feat/database-docs branch 2 times, most recently from 09b8a40 to 13cbc8f Compare September 23, 2024 17:36
Copy link

Coverage of commit 13cbc8f

Summary coverage rate:
  lines......: 92.5% (3310 of 3580 lines)
  functions..: 71.4% (1366 of 1912 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/notifications/db/block_waiver.ex                            |66.7%      3|71.4%     7|    -      0
  lib/notifications/db/bridge_movement.ex                         |66.7%      3|71.4%     7|    -      0
  lib/notifications/db/notification.ex                            |66.7%      6|77.8%     9|    -      0
  lib/notifications/db/notification_user.ex                       | 100%      1|80.0%     5|    -      0
  lib/skate/detours/db/detour.ex                                  | 100%      2| 100%     6|    -      0
  lib/skate/settings/db/route_tab.ex                              |66.7%      3|85.7%     7|    -      0
  lib/skate/settings/db/test_group.ex                             | 100%      3|85.7%     7|    -      0
  lib/skate/settings/db/test_group_user.ex                        |66.7%      3|71.4%     7|    -      0
  lib/skate/settings/db/user.ex                                   |66.7%      3|85.7%     7|    -      0
  lib/skate/settings/db/user_settings.ex                          |66.7%      3|85.7%     7|    -      0

Download coverage report

@joshlarson
Copy link
Contributor

@firestack

But on the other hand, putting the @module_doc in the schema macro removes the choice later to easily change the section: :ecto value, I looked into making the @module_doc setting conditional, but wasn't very successful on my first attempt.

I'm not worried about this. If it turns out to be a problem later on, we can remove it from the __using__ section in schema and put it somewhere else.

Copy link
Contributor

@joshlarson joshlarson left a comment

Choose a reason for hiding this comment

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

I went back and forth because the generated docs are pretty sparse. But you did advertise these as stubs, and they do have the potential to fix some broken links.

I'm good with this as-is. I'm also good with you updating the macro itself as you suggested in the PR description.

@firestack firestack force-pushed the kf/feat/database-docs branch 2 times, most recently from b7efcb0 to b709b37 Compare September 24, 2024 16:25
@firestack firestack enabled auto-merge (squash) September 24, 2024 16:26
`mix docs` doesn't generate anything for these modules if `@moduledoc false` is set, but we're using `TypedEctoSchema` now which _does_ generate type information, which can be surfaced by `ExDoc`.

It's also impossible to link to modules in documentation if `@moduledoc false` is set.
should make it easier to find our database related code, and easier to navigate in the documentation
Copy link

Coverage of commit 8d226f3

Summary coverage rate:
  lines......: 92.5% (3310 of 3580 lines)
  functions..: 71.4% (1366 of 1912 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/notifications/db/block_waiver.ex                            |66.7%      3|71.4%     7|    -      0
  lib/notifications/db/bridge_movement.ex                         |66.7%      3|71.4%     7|    -      0
  lib/notifications/db/notification.ex                            |66.7%      6|77.8%     9|    -      0
  lib/notifications/db/notification_user.ex                       | 100%      1|80.0%     5|    -      0
  lib/skate/detours/db/detour.ex                                  | 100%      2| 100%     6|    -      0
  lib/skate/settings/db/route_tab.ex                              |66.7%      3|85.7%     7|    -      0
  lib/skate/settings/db/test_group.ex                             | 100%      3|85.7%     7|    -      0
  lib/skate/settings/db/test_group_user.ex                        |66.7%      3|71.4%     7|    -      0
  lib/skate/settings/db/user.ex                                   |66.7%      3|85.7%     7|    -      0
  lib/skate/settings/db/user_settings.ex                          |66.7%      3|85.7%     7|    -      0

Download coverage report

@firestack firestack merged commit 444a29f into main Sep 24, 2024
9 checks passed
@firestack firestack deleted the kf/feat/database-docs branch September 24, 2024 16:37
Copy link

Coverage of commit 8d226f3

Summary coverage rate:
  lines......: 92.5% (3310 of 3580 lines)
  functions..: 71.4% (1366 of 1912 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/notifications/db/block_waiver.ex                            |66.7%      3|71.4%     7|    -      0
  lib/notifications/db/bridge_movement.ex                         |66.7%      3|71.4%     7|    -      0
  lib/notifications/db/notification.ex                            |66.7%      6|77.8%     9|    -      0
  lib/notifications/db/notification_user.ex                       | 100%      1|80.0%     5|    -      0
  lib/skate/detours/db/detour.ex                                  | 100%      2| 100%     6|    -      0
  lib/skate/settings/db/route_tab.ex                              |66.7%      3|85.7%     7|    -      0
  lib/skate/settings/db/test_group.ex                             | 100%      3|85.7%     7|    -      0
  lib/skate/settings/db/test_group_user.ex                        |66.7%      3|71.4%     7|    -      0
  lib/skate/settings/db/user.ex                                   |66.7%      3|85.7%     7|    -      0
  lib/skate/settings/db/user_settings.ex                          |66.7%      3|85.7%     7|    -      0

Download coverage report

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