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

Add docblock to segment getter/setter methods #115

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jay-knight
Copy link
Contributor

@jay-knight jay-knight commented Dec 16, 2023

This adds docblock to all of the segment getter/setter methods, primarily so that pages like this include the field numbers that they use.

I used this sed script to generate them:

# Add docblock to setters
s/public function set\([A-Za-z]*\)($value, int $position = \([0-9]*\)/\/**\
     * Set \1 (OBR.\2)\
     *\
     * @param string|int|array|null $value\
     * @param int $position Defaults to \2\
     *\
     * @return bool\
     *\
     *\/\
    \0/

# Add docblock to getters
s/public function get\([A-Za-z]*\)(int $position = \([0-9]*\)/\/**\
     * Get \1 (OBR.\2)\
     *\
     * @param int $position Defaults to \2\
     *\
     * @return array|string|int|null\
     *\
     *\/\
    \0/

# Add spaces to camel case descriptions
/\* [GS]et [A-Z][^\n]*[a-z][A-Z]/s/\([a-z]\)\([A-Z]\)/\1 \2/

(Then did some extra touch ups after that)

@senaranya
Copy link
Owner

Thank you for your contribution! Much appreciated!
Can you please keep only those changes that's needed for the purpose of the ticket?

For example,

  1. In this case @param int $position Defaults to 7 and @return bool are redundant as the method signature contains the same information.
    /**
     * Set Resource QuantityUnits (OBR.7)
     *
     * @param string|int|array|null $value
     * @param int $position Defaults to 7
     *
     * @return bool
     *
     */
    public function setResourceQuantityUnits($value, int $position = 7): bool
  1. Also, if possible do a separate ticket/branch/PR first with just the doc (*.md) updates? That way the changes in the docblock updates will remain smaller and focused.
  2. etc..., if any

Since these days the IDEs are powerful enough to show documentation of a method from its signature on-the-fly, we may do away with the static docs at some point. That's part of the reason these are not always updated. Just curious, can you please let me know which IDE do you use, and do you use the static docs for some reason?

@jay-knight
Copy link
Contributor Author

Yes, I can work to split these into smaller PRs.

I usually use vim, and ALE gives it sufficient autocomplete and some method signature when I'm typing a method name, etc. Other people on our team use emacs, vscode, and probably more. But what t I've needed is when someone says that a certain date is in OBR.7, I don't have a way to find the corresponding method name for that field without going into the code. Having a reference that maps between SEG.# and method names will be helpful.

@senaranya
Copy link
Owner

That does make sense. Thank you for sharing.
Please split it into smaller PRs if you don't mind...

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