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

Fix various bugs/issues in Dancer2 Pod #1688

Closed
wants to merge 3 commits into from
Closed

Conversation

cromedome
Copy link
Contributor

Several minor fixes that seemed silly to put on separate PRs.

I'm not sure if dropping the pod for runner() will be controversial or not. I don't think it fits with the rest of this information though.

The actual text displayed the mailto: which looked a little funky.
In the context of all the other information we are presenting via the
README, the Pod for runner() seems completely out of place. I can drop
this commit if anyone feels the Pod for runner() is necessary, but I
don't think it serves any purpose here. Change my mind!
@xsawyerx
Copy link
Member

xsawyerx commented Sep 1, 2023

What's the reason for removing the pod for runner?

@cromedome
Copy link
Contributor Author

cromedome commented Sep 2, 2023

Look at our README - it's full of a bunch of good information about what Dancer2 is, where to find different docs, how to get help, etc. And then you have what seems to be one random function thrown in the middle. It seems out of place.

Now, consider that this is also the pod when you run perldoc Dancer2 - it makes quite a lot more sense.

This is a situation of having my cake and wanting to eat it too. I don't really like seeing the runner pod in the README, but I think it belongs in the pod. We could not autogenerate the README and keep a separate one, but now we'd be maintaining the same information in two places - the README and Dancer2.pm. That's not great.

I'm not sure we're able to not include the runner docs when auto-generating the README.

So that's where this comes from. Perhaps the solution is worry about actually important things?

Copy link
Contributor

@yanick yanick left a comment

Choose a reason for hiding this comment

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

💯

@GeekRuthie GeekRuthie self-requested a review September 8, 2023 13:10
@cromedome
Copy link
Contributor Author

I suppose my other objection to keeping Pod for runner() it's the only thing documented in Dancer2.pm. If you look at distributions like DBIx::Class and Mojolicious (or frameworks from other languages), they follow this convention too.

I promise I am not trying to argue this; your comments @xsawyerx forced me to justify this change to myself as much as anything :)

Given the approvals, I am merging this. That being said, I am happy to reverse or reopen this issue at any time.

@cromedome cromedome closed this Sep 9, 2023
@cromedome cromedome deleted the docs/fix-various-bugs branch September 9, 2023 13:59
cromedome added a commit that referenced this pull request Oct 9, 2023
    [ BUG FIXES ]
    * GH #1663: Allow overriding of prefix in add_route (GeekRuthie)
    * GH #1675: Stringify VERSION_FROM correctly in Makefile.PL (Jason
      A. Crome)
    * GH #1677: Don't deserialize multipart form data on post (Emil
      Perhinschi)
    * GH #1694: Update JS assets in Dancer2 app skel (Jason A. Crome)

    [ ENHANCEMENTS ]
    * PR #1682: Bump minimum version of Perl to 5.14 (Jason A. Crome)

    [ DOCUMENTATION ]
    * GH #1580: Document the purpose of the .dancer file (Jason A. Crome)
    * GH #1669: Show correct usage of Dancer2::Core::Error (GeekRuthie)
    * GH #1674: Fix POD for input_handle() (mauke)
    * GH #1414: Add documentation resources to the doc map (Jason A.
      Crome, Yanick Champoux)
    * PR #1684: Remove shumphrey from core developers (Steven Humphrey)
    * GH #1685: Document the versioning scheme and Dancer2 release
      process (Jason A. Crome)
    * PR #1688: Fixed various bugs/issues in Dancer2 Pod (Jason A. Crome)
    * PR #1691: Update the contribution guidelines (Jason A. Crome)
    * PR #1692: Change README extension .mkdn -> .md (Jason A. Crome)

    [ DEPRECATED ]
    * GH #1645: Deprecated Dancer2::Test (Jason A. Crome)
    * GH #1646: Deprecated keyword: push_header (Jason A. Crome)
    * GH #1647: Deprecated keyword: header (Jason A. Crome)
    * GH #1648: Deprecated keyword: headers (Jason A. Crome)
    * GH #1649: Deprecated keyword: context (Jason A. Crome)
    * GH #1650: Deprecated: splat/capture named placeholders (Jason A.
      Crome)
    * GH #1651: Deprecated core Request instance method:
      request->dispatch_path (Jason A. Crome)
    * GH #1652: Deprecated keyword in plugins: plugin_setting (Jason A.
      Crome)
    * GH #1653: Deprecated keyword in plugins: dancer_app (Jason A. Crome)
    * GH #1654: Deprecated keyword in plugins: request (Jason A. Crome)
    * GH #1655: Deprecated keyword in plugins: var (Jason A. Crome)
    * GH #1656: Deprecated keyword in plugins: hook (Jason A. Crome)

    [ MISC ]
    * GH #1659: Rename `master` branch to be `main` (Yanick Champoux)
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.

4 participants