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

v2.0.7: inconsistencies in SedStyle #67

Open
agarny opened this issue Oct 8, 2019 · 4 comments
Open

v2.0.7: inconsistencies in SedStyle #67

agarny opened this issue Oct 8, 2019 · 4 comments

Comments

@agarny
Copy link
Contributor

agarny commented Oct 8, 2019

SedLine-related things in SedStyle used to be named xxxLine(), but with the release of v2.0.7 they are now named xxxLineStyle(). I can see why (to avoid conflicts with SedBase's getLine() method. However, two issues:

  • SedStyle defines SedLine* mLine, which shadows SedBase::mLine. So, I would expect SedStyle::mLine to be renamed to SedStyle::mLineStyle.
  • The renaming of xxxLine() to xxxLineStyle() makes xxxMarker() inconsistent. So, I would expect xxxMarker() to be renamed to xxxMarkerStyle() and SedStyle::mMarker to SedStyle::mMarkerStyle.
@fbergmann
Copy link
Owner

I'm also not happy about the renaming, and if you have a better name, then that would be fine. I also dislike the redundancy of the Style object having getXXXStyle methods. But sure, i can rename the remaining things as well for 2.0.8 if you prefer.

@agarny
Copy link
Contributor Author

agarny commented Oct 9, 2019

I'm also not happy about the renaming, and if you have a better name, then that would be fine.

Agreed. Unfortunately, I can't think of a better name at this stage.

I also dislike the redundancy of the Style object having getXXXStyle methods.

Heartily agreed with that one too.

But sure, i can rename the remaining things as well for 2.0.8 if you prefer.

Yes, although I don't like the current names, I am also in favour of consistency... :)

@luciansmith
Copy link
Collaborator

I think we decided to stick with the current names--can this be closed?

@matthiaskoenig
Copy link

I am good with the way it is right now. Can be closed from my side.

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

No branches or pull requests

4 participants