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

Suggestion to refactor apollo_ prefixed methods in ApolloCodegenLib #1170

Closed
yuzushioh opened this issue Apr 24, 2020 · 7 comments
Closed

Suggestion to refactor apollo_ prefixed methods in ApolloCodegenLib #1170

yuzushioh opened this issue Apr 24, 2020 · 7 comments
Labels
enhancement Issues outlining new things we want to do or things that will make our lives as devs easier
Milestone

Comments

@yuzushioh
Copy link
Contributor

yuzushioh commented Apr 24, 2020

Hi @designatednerd

I have been using apollo-ios for about 6 months and I love it. I just migrated the code generation script from apollo-tooling to Swift scripting using ApolloCodegenLib.

While I was doing a work, I noticed some of framework's methods have apollo_ prefix like apollo_folderExists. I think it is there to make it more recognizable for users that those are only used for apollo's usecases.

RxSwift used to use the same pattern like rx_text but it has been changed to rx.text by using its own extension. I'm thinking It will be a good change in Apollo too. I can do the work. What do you think?

@designatednerd
Copy link
Contributor

Yeah the namespacing is a) To make it clearer that this is something that Apollo added and b) To prevent accidental runtime naming clashes on anything that has to go through the Obj-C runtime (which most stuff descended from NSObject has to do).

I was trying to remember how Rx did this and I found the Reactive type in their codebase - it's not quite as involved as I remember it being.

I'm a little torn - i think the underscore methods make it easier to discover what apollo_blah methods/properties exist in one go for autocomplete, but it does read nicer to have apollo.blah, and it seems like the maintenance burden of switching to this style shouldn't be too terrible.

I'd say go ahead and open a PR - I think I'd still like to use apollo for the namespace but I think otherwise the Reactive stuff seems like it provides a good template to use.

@designatednerd designatednerd added the enhancement Issues outlining new things we want to do or things that will make our lives as devs easier label Apr 24, 2020
@yuzushioh
Copy link
Contributor Author

@designatednerd Hi, is there any reason why the indentation width is set for 2 tab and 2 space? I personally think it's more common to use 4 for indentation width.

@designatednerd
Copy link
Contributor

2-space indentation was set up well before I started on this project. To be honest, I don't really care enough either way to change it, so I'm going with what we were already using.

@yuzushioh
Copy link
Contributor Author

Are you going to use any linting framework like SwiftLint in this project at some point?

@designatednerd
Copy link
Contributor

At some point I'd like to, but it's not a super-high priority since at this point I'm the benevolent dictator primary maintainer of the code. What, in particular, were you wondering about trying to enforce?

@yuzushioh
Copy link
Contributor Author

My Xcode text setting's default indentation style is set for 4 spaces so wherever I write a piece of code it will be indented as 4 spaces and its not the same as apollo's indentation style (which is 4 spaces).

But I opened the apollo project in this morning and its indentation style is set for 2 spaces so it's fine now. Sorry for confusing you here.

@yuzushioh
Copy link
Contributor Author

Fixed by #1183

@designatednerd designatednerd added this to the Next Release milestone May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues outlining new things we want to do or things that will make our lives as devs easier
Projects
None yet
Development

No branches or pull requests

2 participants