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.

Sign up for GitHub

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

Jump to bottom

Add apply method for constructing CustomTag and CustomAttributes #318

Merged
merged 1 commit into from
Jan 27, 2020
Merged

Add apply method for constructing CustomTag and CustomAttributes #318

merged 1 commit into from
Jan 27, 2020

Conversation

joan38
Copy link
Contributor

@joan38 joan38 commented Jan 21, 2020 •
edited
Loading

I couldn't think of a case where we would need multiple instances of these with the same name, so it sounds like it should be case classes.

@shadaj
Copy link
Owner

shadaj commented Jan 21, 2020

Since the name property is private, which was done intentionally to minimize the user-facing API surface, I'm not super sure why these need to be case classes since there would be nothing to extract. Could you explain the idea behind this PR?

@joan38
Copy link
Contributor Author

joan38 commented Jan 21, 2020

As agreed on Gitter, I'm doing an apply in the companion and we'll think about it.

@shadaj shadaj force-pushed the master branch 8 times, most recently from 29fe37b to 4d7136c Compare January 22, 2020 03:36
shadaj
shadaj requested changes Jan 26, 2020
View reviewed changes
Copy link
Owner

@shadaj shadaj left a comment

Choose a reason for hiding this comment

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

Code LGTM! I think the apply method should probably be the recommended style going forward, so could you update the docs to use it?

@joan38
Copy link
Contributor Author

joan38 commented Jan 27, 2020

Comments addressed

shadaj
shadaj requested changes Jan 27, 2020
View reviewed changes
Copy link
Owner

@shadaj shadaj left a comment

Choose a reason for hiding this comment

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

One last thing (apologies for the repeated back-and-forth), could you add a line to the changelog for this?

@shadaj shadaj changed the title Make CustomTag and CustomAttributes case classes Add apply method for constructing CustomTag and CustomAttributes Jan 27, 2020
@joan38
Copy link
Contributor Author

joan38 commented Jan 27, 2020

No worries. Fixed

@shadaj shadaj merged commit 0cb6f01 into shadaj:master Jan 27, 2020
@shadaj shadaj added this to the v0.6.4 milestone Jan 31, 2020
@joan38 joan38 deleted the case-class branch February 3, 2020 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shadaj shadaj shadaj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.6.4
Development

Successfully merging this pull request may close these issues.
2 participants
@joan38 @shadaj