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

Add client to codegen-client package namespace #1711

Merged
merged 4 commits into from
Sep 9, 2022

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented Sep 7, 2022

Motivation and Context

I didn't want to include this in #1697 since it would make that diff a lot larger and more difficult to merge in. This adds client to the package names in the codegen-client module.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@david-perez
Copy link
Contributor

@david-perez
Copy link
Contributor

Are we settling then on the convention that the namespace containing client or server is enough to distinguish to which project a class belongs to? In the server project we've also been prefixing most of our classes with Server to tell at a glance, so that readers don't have to inspect the classpath.

@jdisanti
Copy link
Collaborator Author

jdisanti commented Sep 9, 2022

Perhaps we should ignore this commit in blame? https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

I like this idea! Looks like I'll have to do this after merging this since the final squashed commit hash isn't known yet.

Are we settling then on the convention that the namespace containing client or server is enough to distinguish to which project a class belongs to? In the server project we've also been prefixing most of our classes with Server to tell at a glance, so that readers don't have to inspect the classpath.

I wasn't intending to establish anything one way or another here, but I think this should be fine if you want to do it. The only downside is that it will be harder to find the class you want in an IDE when searching by name since you'll have multiple to choose from. I'm hoping we'll have less name collisions as we refactor away from an inheritance approach to a composition approach.

@jdisanti jdisanti enabled auto-merge (squash) September 9, 2022 16:59
@github-actions
Copy link

github-actions bot commented Sep 9, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti merged commit 1a0f81a into main Sep 9, 2022
@jdisanti jdisanti deleted the jdisanti-client-package branch September 9, 2022 17:32
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.

4 participants