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: add quotes to automatically generated condition contexts on parent #1184

Merged
merged 1 commit into from
May 23, 2024

Conversation

florianesser
Copy link
Member

@florianesser florianesser commented May 22, 2024

When generating a condition context on a parent for a property with a name that has special meaning in CQL (like id), the generated filter does not work as expected. With this change, the property name is put in quotes so that filters with special property names work correctly.

Closes #1104

Copy link

github-actions bot commented May 22, 2024

hale studio builds for this pull request:

Build triggered for commit c34f6b1.
Artifacts are retained for 14 days.

Copy link
Member

@stempler stempler left a comment

Choose a reason for hiding this comment

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

I think for the link to the issue showing up properly in the changelog the reference needs to be prefixed with "closes" or "fixes".

e.g. Closes #1104

@florianesser
Copy link
Member Author

I think for the link to the issue showing up properly in the changelog the reference needs to be prefixed with "closes" or "fixes".

@stempler Do you mean in the commit message or in the here in the PR description?

My reasoning for not adding a closes reference was to avoid that GitHub closes the issue automatically as soon as the PR is merged. If we're ok with that, I'm happy to add it.

@stempler
Copy link
Member

@florianesser Not 100% sure, I think it should be at least in the commit. We disussed that recently (see here). I think preferable would be if the PR is merged after the issue is challenged, which ideally can be done with the artifacts provided in the PR. What do you think?

@emanuelaepure10
Copy link
Contributor

From what I know the keywords that can be used are the following:

  • Closes #issue_number
  • Fixes #issue_number
  • Resolves #issue_number

These can be added as well to our "Developer Guidelines".

@florianesser
Copy link
Member Author

I think preferable would be if the PR is merged after the issue is challenged, which ideally can be done with the artifacts provided in the PR. What do you think?

Ok, I see, I'll add the keyword. I'll just have to remember not to merge directly after approval of the PR anymore :)

@florianesser
Copy link
Member Author

Might be a nice enhancement to add a comment with the build artifacts directly to the referenced issue as well. Or was that not done on purpose?

@stempler
Copy link
Member

Might be a nice enhancement to add a comment with the build artifacts directly to the referenced issue as well. Or was that not done on purpose?

Do you mean when the PR is merged?

semantic-release will add a link to the GitHub release (see example here), and the artifacts there are available permanently.

The artifacts for the PR are only available temporarily, and may be updated.
For adding a comment to the issue we would also need some logic to determine the issue, not sure if that's easily done.

@stempler
Copy link
Member

From what I know the keywords that can be used are the following:

See here for what is supported by default by the release process.

@florianesser
Copy link
Member Author

Do you mean when the PR is merged?

No, I meant when the PR artifacts are built so that the challenger can see them directly in the issue (I now mentioned them manually there), but...

For adding a comment to the issue we would also need some logic to determine the issue, not sure if that's easily done.

that explains it. As the PR is referenced anyway if one of the "Closes" keywords is used, I don't think it's necessary to put effort into an automatic comment.

When generating a condition context on a parent for a property with a name that
has special meaning in CQL (like `id`), the generated filter does not work as
expected. With this change, the property name is put in quotes so that filters
with special property names work correctly.

Closes #1104
@florianesser florianesser force-pushed the fix/id-condition-filter-on-parent branch from 05168e2 to e520af3 Compare May 23, 2024 07:36
@florianesser
Copy link
Member Author

Implementation was challenged by Johanna, will merge as soon as the checks are done.

@florianesser florianesser merged commit 72b243d into master May 23, 2024
3 checks passed
@florianesser florianesser deleted the fix/id-condition-filter-on-parent branch May 23, 2024 08:00
Copy link

we-release bot commented Jun 19, 2024

🎉 This PR is included in version 5.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@we-release we-release bot added the released label Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: Condition context filter created via "Occurring values" in Properties view is not valid
3 participants