Remove non-identifier characters from ruby_names #78

Merged
merged 2 commits into from Feb 15, 2017

Projects

None yet

4 participants

@brentdax
brentdax commented Jan 3, 2017 edited

If ruby_name is passed a string with characters that are not dashes, spaces, or in \w, this patch will remove those characters entirely from the string.

To be clear, I'm not sure this is the design we want—think of this as a way of prompting discussion.

I've put this in because the Heroku API schema currently has a link with the title "Create (Deprecated)". Heroics was trying to expose this as a method called create_(deprecated), which is obviously invalid. With this patch, it instead becomes create_deprecated, which is probably the best mapping we can expect.

(There are other problems with the Platform API schema—some links are missing titles entirely—but I have a ticket filed with Heroku to get that fixed.)

Brent Royal-Gordon Remove non-identifier characters from ruby_names
This deals with a link titled “Create (Deprecated)” in the current Heroku Platform API schema, turning it into `create_deprecated`.
7960d3d
@geemus
Member
geemus commented Jan 4, 2017

@brentdax hey, thanks for bringing these up. Seems like maybe we should just fix the schema? At least see how far that can get us. I haven't seen the other ticket, if you link me to it I can respond more directly. Thanks!

@brentdax
brentdax commented Jan 4, 2017

@geemus Here's the ticket. I don't disagree that the schema should be fixed, but there's no reason not to give this gem some sort of sensible behavior when it encounters a character that doesn't belong in an identifier, even if it's just raising an error.

@rwz
rwz commented Jan 4, 2017

Here's the PR fixing the schema: heroku/api#7245

@geemus
Member
geemus commented Jan 5, 2017

@rwz great, thanks!

@geemus
Member
geemus commented Jan 5, 2017

@brentdax that makes sense. Having it change under the covers seems like it might be surprising/weird in some cases, but I can definitely see raising an error being a quick/helpful way to see something has gone awry. How do you feel about that as our first pass instead?

@brentdax

@geemus I would be quite happy to see an error raised. It'd be nice if it included enough information to locate the offending part of the schema—at least the string with the invalid character, and perhaps even the associated href property if that's not too difficult.

@rwz I don't work at Heroku so I can't see the pull request's status, but for what it's worth, the schema fetched from the API in production still appears to include the (deprecated) name.

Brent Royal-Gordon Raise error when name contains invalid chars
Consensus in the pull request was that an error would be a better approach.
e4bc2a2
@brentdax

I pushed a commit which throws a SchemaError with a message identifying both the original name and the invalid Ruby name. Adding more details would probably require subclassing SchemaError and rescuing in the caller to add additional context, which seems a little heavyweight. The check is inelegant, but the best alternative I thought of was using tap, which felt like it was too clever by half.

@geemus
Member
geemus commented Jan 20, 2017

@brentdax looks like a good first pass at this. We'll be sure to sort out the broader schema issues too, though I'm not sure of the exact time table. Thanks for the help and your patience as we work through this.

@mathias
Contributor
mathias commented Jan 25, 2017

@geemus I think we still need something that keeps the platform-api gem from generating methods like organization_add_on when it was previously named organization_addon for compatibility. Should we reduce this to just removing the hyphens?

@geemus
Member
geemus commented Jan 26, 2017

@mathias quite possibly, though I worry a bit that we are now overfitting (I'm not sure that is a generally applicable pattern, vs just something that happens to matter for us due to this particular situation). What do you think?

@mathias
Contributor
mathias commented Jan 26, 2017

@geemus if we bump platform-api as a breaking change, then I'm fine with the method names getting _ for -. I am not sure how many people that's going to affect, versus changing how Heroics behaves. Do we have an idea of relative user bases?

@mathias
Contributor
mathias commented Jan 26, 2017 edited

@geemus @brentdax An alternate proposal that I came up with: What if we allow a list of substitutions for the ruby_name method in a config file?

This supposes that we're OK with the idea of adding a config file that heroics-generate would read when run (and mirrors the command line options available so that we can simplify repeatable client generation.) Seems like it could work.

@geemus
Member
geemus commented Jan 31, 2017

@mathias I think I'm good with this error style in the PR, is that going to be immediately problematic from our schema though?

@mathias
Contributor
mathias commented Feb 6, 2017

@geemus sorry that I missed this question. If we merge this as-is, the error is fine (and an improvement.

What it changes is the ruby_name behavior in platform-api for our existing methods -- def addon_method becomes def add_on_method -- so #80 and a later PR that introduces a list of regex replacements will allow us to define our own replacements and others using heroics like @brentdax can also define a list of replacements, without trying to force all these things to confirm to exactly the same naming conventions.

@geemus
Member
geemus commented Feb 6, 2017

@mathias I think that sounds reasonable, do we need to stage/wait for this so the timing works out with the required changes on platform-api then?

@mathias
Contributor
mathias commented Feb 6, 2017

@geemus Sorry that I wasn't clear. We need these changes + #80 + a new PR that allows replacement patterns to be configured through Configuration class in #80 before we can update and release platform-api gem again.

@mathias mathias merged commit e4bc2a2 into interagent:master Feb 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment