Skip to content

Slight API change #12

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

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

Slight API change #12

wants to merge 2 commits into from

Conversation

slior
Copy link
Contributor

@slior slior commented Aug 27, 2018

Hi,
This suggest what i think can be an API improvement.

  1. The VerbalExpression case class becomes final - a simple "safety" best practice and shows the intention of this not being extended.
  2. The add method becomes private. I saw it used only internally, and figured it's not intended to be an API, since it basically kind of exposes implementation detail, and might be dangerous to expose as an API (it receives the string to concatenate).
    Note that it's still available for tests.
  3. The start and end of line - instead of having a single API method with boolean parameter - have more explicit methods that wrap those single method and provide a more explicit expression of the intent.

Let me know what you think.

slior added 2 commits August 15, 2018 22:53
also making 'VerbalExpression' final since it's not extended
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