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

Update Generated extension names to better reflect their contents #116

Merged
merged 5 commits into from
Aug 7, 2022

Conversation

liamnichols
Copy link
Member

When looking at the generated output, I didn't get the feeling that the extension files were named very naturally.

For example, a file containing the StringCodingKey type is named Entities+CodingKey.swift which to me feels a bit disconnected. If I was naming it myself, I'd have gone with StringCodingKey.swift instead.

In the end, I decided to rename the all of the files to better reflect their contents:

Before After
Entities+CodingKey.swift StringCodingKey.swift
Entities+AnyJSON.swift AnyJSON.swift
Paths+Extensions.swift Paths.swift (actually options.paths.namespace)

WDYT @LePips ?

@LePips
Copy link
Contributor

LePips commented Aug 5, 2022

Perfect 👍

Edit: actually Paths.swift will conflict with a --merged generation since the Paths file is Paths.swift

@liamnichols
Copy link
Member Author

liamnichols commented Aug 5, 2022

Edit: actually Paths.swift will conflict with a --merged generation since the Paths file is Paths.swift

Oof good point! Now I'm wondering why the tests didn't flag that 😄

Edit: Ah, it overwrote it 😄

Screenshot 2022-08-05 at 22 18 54

@liamnichols
Copy link
Member Author

I wonder if its easiest here to just go back on what we said earlier and continue to embed the extensions inside the merged file? I'm trying to remember why I suggested that and I think the only real reason was because I thought it would be easier and would push people towards not merging sources.

Any concerns with that? Alternatively I could think of a better name, but I've got nothing.

@LePips
Copy link
Contributor

LePips commented Aug 5, 2022

I think that's good for merged. However, I think there's a good probability that a few schemas exist that have a Paths object which would conflict. We can ignore that for now and wait until somebody raises an issue.

@liamnichols
Copy link
Member Author

Yeah at a minimum the generator should probably spot that conflict and error, but I'm looking at the file output formatting code now and again I think we could doing some refactoring in this area to clean things up nicely.

I'll merge them in for the time being but will keep this in mind 👍

@liamnichols
Copy link
Member Author

Actually, an object called Paths would be written to ./Paths/Paths.swift when not merging, but the Paths namespace would be written to ./Paths.swift.. So while the files wouldn't overwrite each other, the two types would conflict and the user would have to either change the namespace or rename the entity 😄

@LePips
Copy link
Contributor

LePips commented Aug 5, 2022

Actually, what if we changed the namespace to be the name of the package/module by default? This would only affect paths but that's a correct expectation. The directory Paths/ would stay the same as it holds paths.

import PetStoreAPI

let petsRequest = PetStoreAPI.getPets()

Edit: scratch that, there's a bug in the compiler when using the same name as the module for namespacing purposes: apple/swift#43510. I remembered OpenAPITools works around this issue by making names somewhat dumb. Wouldn't hurt to make sure though.

@kean
Copy link
Member

kean commented Aug 5, 2022

Edit: Ah, it overwrote it 😄

Wasn't possible with "+" in file names 😄

@liamnichols
Copy link
Member Author

Reverted the behaviour when using --merge-sources so that it worked like it did before #95 (extensions are now included in the merged sources again).

They're only placed outside of the Entities/Paths group when generating them in split mode. It avoids naming collisions and lets us stick with more natural names

@liamnichols liamnichols merged commit b0eddb4 into main Aug 7, 2022
@liamnichols liamnichols deleted the ln/rename-extensions branch August 13, 2022 16:09
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.

None yet

3 participants