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

Clean up byline validation #905

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Clean up byline validation #905

wants to merge 2 commits into from

Conversation

JKingweb
Copy link
Contributor

The _checkByline function is a little weird in a few ways:

  • Despite its name and return type it does more than check
  • It relies on external information to short-curcuit itself
  • It calls another function to do more checks
  • It checks whether the node is an element (which will always be true) before getting attributes, but unconditionally relies on variables it has set conditionally

It gets the job done, but I found it confusing to read and make sense of considering its modest length. I have consequently cleaned it up and re-organized the logic surrounding byline harvesting in general.

I included some basic documentation, but since I've never written a JavaScript doc-string before and I'm uncertain what the matchString argument is actually for, suggestions for improvement there are welcome.

For reference, one can assume that node is an element becaause the _grabArticle loop begins at documentElement, and the _getNextNode function which drives the loop always returns an element if it returns a node.

- Consolidate _checkByline and _isValidByline into one function
- Avoid side effects and short circuiting
- Move pre-existence checks and side effects out to _grabArticle
- Add basic documentation
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.

1 participant