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

split packages into their own files #43

Merged
merged 1 commit into from
Sep 14, 2022
Merged

split packages into their own files #43

merged 1 commit into from
Sep 14, 2022

Conversation

simbabque
Copy link
Contributor

No description provided.

@simbabque simbabque requested a review from oalders August 24, 2022 21:46
@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Base: 84.64% // Head: 84.64% // No change to project coverage 👍

Coverage data is based on head (ca88d7f) compared to base (ca88d7f).
Patch has no changes to coverable lines.

❗ Current head ca88d7f differs from pull request most recent head cffb9e7. Consider uploading reports for the commit cffb9e7 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #43   +/-   ##
=======================================
  Coverage   84.64%   84.64%           
=======================================
  Files           1        1           
  Lines         521      521           
  Branches      165      165           
=======================================
  Hits          441      441           
  Misses         39       39           
  Partials       41       41           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@genio
Copy link
Member

genio commented Aug 24, 2022

Can we merge the pod and pm back together? pod at the end of the file is always good.

Copy link
Member

@oalders oalders left a comment

Choose a reason for hiding this comment

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

Looks great! One thing that comes to mind is that it will be harder to follow the history of changes from files that used to be in Form.pm, but it's not an impossible problem to solve.

@simbabque
Copy link
Contributor Author

Can we merge the pod and pm back together? pod at the end of the file is always good.

The POD is a tricky problem. I think it was an important step towards more modern, maintainable code to split the packages out. But the POD is annoying, because the structure of the documentation is really unusual. I like the way it can be perused on metacpan, you can just search in it, and usually find the right thing. But on the other hand a lot of it is undocumented. There are loads of methods that have no POD.

I considered writing POD for all of the new files, but that would have fragmented the documentation, making it harder to use. I discussed this with @oalders and we agreed that it would be a good middle ground to have it in a separate file. This way it's consistent across all of it. It does mean the POD is not near the methods any more, but at least that's the case for all of them.

Having a POD file for all of the docs seems to be a common paradigm on CPAN.

Can you explain why you'd have it all in Form.pm? Also, do you mean the entire doc at the end, or just the extra bits, with the rest where they belonged?

@haarg
Copy link
Member

haarg commented Aug 24, 2022

My preference would be to have the entire pod document at the end of lib/HTML/Form.pm.

The split files are missing a newline on the last line.

@simbabque
Copy link
Contributor Author

Sorry, I was away for a few days. So the consensus seems to be to stick it all at the end of the main pm file? That's fine with me. I'll leave another day or two for comments, and then do that, unless someone has another suggestion.

@oalders
Copy link
Member

oalders commented Aug 30, 2022

So the consensus seems to be to stick it all at the end of the main pm file?

Right, we discussed this on IRC as well and that was the consensus.

@simbabque
Copy link
Contributor Author

How is this?

Changes Outdated Show resolved Hide resolved
@oalders
Copy link
Member

oalders commented Sep 8, 2022

I think once Changes has been updated, this is fine to merge.

@oalders oalders merged commit 7bcc149 into master Sep 14, 2022
@oalders oalders deleted the simbabque/split branch September 14, 2022 11:04
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.

None yet

4 participants