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

Chore: Use project name from context to get settings #354

Merged
merged 3 commits into from
Apr 3, 2024

Conversation

iLLiCiTiT
Copy link
Member

Changelog Description

Use "projectName" value to get settings instead of get_current_project_settings.

Additional info

This is technical change, we should make sure the project which is set in context is used as source of truth for publishing.

Testing notes:

  1. Settings are collected as before.

@iLLiCiTiT iLLiCiTiT requested review from BigRoy and kalisp April 3, 2024 10:43
@ynbot ynbot added size/XS type: enhancement Improvement of existing functionality or minor addition labels Apr 3, 2024
@BigRoy
Copy link
Collaborator

BigRoy commented Apr 3, 2024

The setting of context.data["projectName"] here happens in an if statement that doesn't seem all too promising that we can rely on the project name being accessible.

Same goes for the logic here.

Should that if statement be removed as well?

Testing of this PR should likely need 'special testing' in hosts that still use the legacy publisher since it would go through slightly different logic.

@iLLiCiTiT
Copy link
Member Author

The setting of context.data["projectName"] here happens in an if statement that doesn't seem all too promising that we can rely on the project name being accessible.

Not sure what to do with this, maybe just use .get(...)?

Should that if statement be removed as well?

Which if statement?

@BigRoy
Copy link
Collaborator

BigRoy commented Apr 3, 2024

Which if statement?

The if project_name statements in the code I linked.

project_name = create_context.get_current_project_name()
if project_name:
context.data["projectName"] = project_name

if not project_name:
context.data["projectName"] = current_context["project_name"]

Both seem to 'hint' that context.data["projectName"] MAY not exist.

@iLLiCiTiT
Copy link
Member Author

Both seem to 'hint' that context.data["projectName"] MAY not exis

Yes, I'm asking how to handle that in this plugin. I won't touch the other plugins.

@BigRoy
Copy link
Collaborator

BigRoy commented Apr 3, 2024

Yes, I'm asking how to handle that in this plugin. I won't touch the other plugins.

Ah good question - it seems like we always expected a project name to be 'retrievable' and thus be valid - so I'd actually say we should just make those always being set in the other plug-ins accordingly. Rather prefer that, then now still adding a get_current_project_entity fallback in this plug-in.

@iLLiCiTiT
Copy link
Member Author

iLLiCiTiT commented Apr 3, 2024

it seems like we always expected a project name to be 'retrievable' and thus be valid - so I'd actually say we should just make those always being set in the other plug-ins accordingly.

You can have completelly different logic in custom addon which collects "projectName". So I don't think we should enforce it.

Rather prefer that, then now still adding a get_current_project_entity fallback in this plug-in.

I don't think that is correct behavior. Using current project settings would be against what is inside publish context...

All other plugins are expecting the project is set, it would crash much sooner than in this plugin, and I don't think we should try to handle that case to be honest.

Copy link
Member

@kalisp kalisp left a comment

Choose a reason for hiding this comment

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

Tried a couple of DCCs, didnt explode.

@kalisp kalisp assigned iLLiCiTiT and unassigned kalisp Apr 3, 2024
@iLLiCiTiT iLLiCiTiT merged commit 1bba097 into develop Apr 3, 2024
9 of 10 checks passed
@iLLiCiTiT iLLiCiTiT deleted the enhancement/use-project-name-from-context branch April 3, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS type: enhancement Improvement of existing functionality or minor addition
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants