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

Make template_format checking use a whitelist #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gerrywastaken
Copy link

Code such as the follow contains :html in the
template_formats list, but is made invalid if a html
comment is appended to it.

render(
  partial: 'scss partial',
  locals: { main_color: '#f0f' },
  formats: :scss
)

I've changed the template_format checking code so that
it only applies the template when we are absoutely
certain that the template is in the expected format.

Code such as the follow contains :html in the
template_formats list, but is made invalid if a html
comment is appended to it.

```ruby
render(
  partial: 'scss partial',
  locals: { main_color: '#f0f' },
  formats: :scss
)
```

I've changed the template_format checking code so that
it only applies the template when we are absoutely
certain that the template is in the expected format.
@gerrywastaken
Copy link
Author

Do you see any issues with this change?

@duncanbeevers
Copy link
Owner

@gerrywastaken I haven't touched this gem in several years.

The change seems reasonable but I don't have any day-to-day interaction with the gem by which to measure the value of the change.

@gerrywastaken
Copy link
Author

I'm not sure where to go from here, but what you have said is very understandable. Thanks for looking over the change anyway.

@duncanbeevers
Copy link
Owner

@gerrywastaken
I see three paths forward

  • Copy the gem code into your app's codebase with the changes you want; no more gem dependency
  • Fork the gem and publish your own differently-named version and use that.
  • I can hand over the keys to the gem if you'd like to simply take over. Then you could publish your own "official" version and use that in your app.

@gerrywastaken
Copy link
Author

I'm thinking either option 2 or 3. What would you prefer?

@duncanbeevers
Copy link
Owner

@gerrywastaken My apologies; this fell off my radar.
If you would still like an ownership role on the rails_view_annotator gem, let me know which email address you'd like given ownership, and I'll take care of it.

@gerrywastaken
Copy link
Author

@duncanbeevers No worries at all. You can send it to

['rubygems', 'caulfield.me'].join('@')

;-)

@duncanbeevers
Copy link
Owner

@gerrywastaken In order to become a gem owner, you must have a rubygems.org account registered with the given email address.

@suan
Copy link

suan commented Oct 4, 2018

@gerrywastaken Could u give me ownership so I can merge this fix? My email is ['yeosuanaik', 'gmail.com'].join('@')

Actually, would you mind also giving commit access to this repo? That should make things easier too

@gerrywastaken
Copy link
Author

gerrywastaken commented Oct 12, 2018

@suan Oh I'm not a maintainer yet.

@duncanbeevers Super sorry I missed your first comment. Suan's comment alerted me to it.

It looks like I got the email slightly wrong. It's actually:
['rubygems.org', 'caulfield.me'].join('@')

Or you could add @suan instead.

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.

3 participants