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

fix: Use the fully-qualified names from .definitions instead of namespace #60

Merged

Conversation

aramovic79
Copy link
Contributor

@aramovic79 aramovic79 commented Sep 18, 2024

Addressing the following issue from #38 :

Never use CSN's .namespace property but always just use the fully-qualified names from .definitions

Instead of csn.namespace, we should use e.g. cds.model.definitions["AdminService"].model.namespace

@aramovic79 aramovic79 changed the title fix: Use use the fully-qualified names from .definitions instead of namespace fix: Use the fully-qualified names from .definitions instead of namespace Sep 19, 2024
lib/defaults.js Outdated Show resolved Hide resolved
lib/ord.js Show resolved Hide resolved
lib/ord.js Show resolved Hide resolved
lib/ord.js Outdated Show resolved Hide resolved
@aramovic79 aramovic79 marked this pull request as ready for review September 25, 2024 12:48
@aramovic79
Copy link
Contributor Author

@RoshniNaveenaS @Fannon @zongqichen @RamIndia : Could you please review this pull request? Thanks.

lib/defaults.js Outdated Show resolved Hide resolved
lib/templates.js Outdated Show resolved Hide resolved
lib/templates.js Outdated Show resolved Hide resolved
lib/templates.js Outdated Show resolved Hide resolved
@zongqichen zongqichen linked an issue Sep 25, 2024 that may be closed by this pull request
@aramovic79
Copy link
Contributor Author

@zongqichen @Fannon : Could you please review once more? Thanks.

lib/templates.js Outdated Show resolved Hide resolved
lib/templates.js Outdated Show resolved Hide resolved
@aramovic79
Copy link
Contributor Author

@Fannon @zongqichen : Could you please review?

"packages": [
{
"description": "Description for capire bookshop ord sample",
"ordId": "sap.test.cdsrc.sample:package:capirebookshopordsample-api:v1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks it could generate multi packages by appending different suffix, I guess it's valid @Fannon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so does it now append suffixes or not? I thought for now, it just generates one package and assigns all the ORD resources to it? But in the future, we would add the option to automatically add the suffixes and assign the resources to the right kind of package :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the original code has the logic to append suffix to different type.

Copy link
Contributor

@zongqichen zongqichen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zongqichen zongqichen merged commit 1f70236 into main Oct 1, 2024
3 checks passed
@zongqichen zongqichen deleted the fix/use-fqn-from-definitions-instead-of-csns-namespace branch October 1, 2024 13:47
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.

Ord generated document contains undefined in different ids
3 participants