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

Generate unique internal var names and add tests for name collisions. #601

Merged
merged 5 commits into from
Oct 16, 2023

Conversation

inickles
Copy link
Contributor

Ok we're back, one last time for now. 🤞

There are two changes here:

  1. @augustuswm suggested that I add the test he'd introduced in the now-closed Query param named "query" collision #261.
  2. In doing so I realized generating a unique name for internal variables from the params wouldn't be hard and it'd create less noise in generated output. I feel better about this than the static prefixes I introduces earlier.

Example output from new test:

impl Client {
    ///Gets a key
    ///
    ///Sends a `GET` request to `/key/{query}`
    ///
    ///Arguments:
    /// - `query`: Parameter name that was previously colliding
    /// - `client`: Parameter name that was previously colliding
    /// - `request`: Parameter name that was previously colliding
    /// - `response`: Parameter name that was previously colliding
    /// - `result`: Parameter name that was previously colliding
    /// - `url`: Parameter name that was previously colliding
    pub async fn key_get<'a>(
        &'a self,
        query: bool,
        client: bool,
        request: bool,
        response: bool,
        result: bool,
        url: bool,
    ) -> Result<ResponseValue<()>, Error<()>> {
        let _url = format!("{}/key/{}", self.baseurl, encode_path(&query.to_string()),);
        let mut _query = Vec::with_capacity(5usize);
        _query.push(("client", client.to_string()));
        _query.push(("request", request.to_string()));
        _query.push(("response", response.to_string()));
        _query.push(("result", result.to_string()));
        _query.push(("url", url.to_string()));
        let _request = self.client.get(_url).query(&_query).build()?;
        let _result = self.client.execute(_request).await;
        let _response = _result?;
        match _response.status().as_u16() {
            200u16 => Ok(ResponseValue::empty(_response)),
            _ => Err(Error::UnexpectedResponse(_response)),
        }
    }
}

@inickles inickles requested a review from ahl October 13, 2023 13:09
@augustuswm
Copy link
Contributor

One small suggestion. Does it make sense here to use an underscore suffix instead of a prefix due to the underscore prefix having a semantic meaning of "may be unused" already? Given that it is all occurring in generated code, I don't think there is a worry of correctness. As a reader of the generated code though, you may wonder why they are all marked that way.

@inickles
Copy link
Contributor Author

One small suggestion. Does it make sense here to use an underscore suffix instead of a prefix due to the underscore prefix having a semantic meaning of "may be unused" already? Given that it is all occurring in generated code, I don't think there is a worry of correctness. As a reader of the generated code though, you may wonder why they are all marked that way.

Yeah, I could see that. Though my general preference for this is for prefixing, the differentiation is clearer to me, so if we wanted to alleviate that potential confusion my preference would be to use a double underscore, which I've seen used for private things.

I could change the string in https://github.com/oxidecomputer/progenitor/pull/601/files#diff-25893ecdd4f4d0e002500c89a463ff6bc8e30149732f3cb90dd1bc247c5e788cR136 to just be a "__", which would increase the prefix by two at a time, still always prefixing by default.

I don't care if we choose to increment by one or two underscores (or perhaps an emoji instead) upon collision, or even if we default to using a prefix from the start - I could go either way on both those points and I'll defer to you @augustuswm and @ahl. Simplest option seems to be just replace the str with "__".

progenitor-impl/src/util.rs Outdated Show resolved Hide resolved
sample_openapi/param-collision.json Show resolved Hide resolved
Co-authored-by: Adam Leventhal <[email protected]>
@inickles inickles force-pushed the inickles/unique_idents branch from d3daa49 to aaa4085 Compare October 13, 2023 20:44
@ahl
Copy link
Collaborator

ahl commented Oct 16, 2023

nice

@ahl ahl merged commit 6dc2a0f into main Oct 16, 2023
5 checks passed
@ahl ahl deleted the inickles/unique_idents branch October 16, 2023 18:44
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