8000 feat(libs/pubsub): allow dash symbol in event type/attribute by melekes · Pull Request #3456 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(libs/pubsub): allow dash symbol in event type/attribute #3456

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 9, 2024
Merged

feat(libs/pubsub): allow dash symbol in event type/attribute #3456

merged 5 commits into from
Jul 9, 2024

Conversation

melekes
Copy link
Contributor
@melekes melekes commented Jul 8, 2024

Closes #3401

PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments
  • Title follows the Conventional Commits spec

@melekes melekes requested review from a team as code owners July 8, 2024 06:52
@melekes melekes mentioned this pull request Jul 8, 2024
Closed
4 tasks
@melekes melekes self-assigned this Jul 8, 2024
@melekes melekes added backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x backport-to-v1.x Tell Mergify to backport the PR to v1.x and removed backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x backport-to-v1.x Tell Mergify to backport the PR to v1.x labels Jul 8, 2024
@melekes melekes enabled auto-merge July 8, 2024 06:58
@melekes melekes disabled auto-merge July 8, 2024 07:21
@melekes melekes added the indexer label Jul 8, 2024
@melekes
Copy link
Contributor Author
melekes commented Jul 8, 2024

cc @Lockwarr

cason
cason approved these changes Jul 8, 2024
View reviewed changes
Copy link
Contributor
@cason cason left a comment

Choose a reason for hiding this comment

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

Looks neat, thank you for updating also the documentation.

@Lockwarr
Copy link
Contributor
Lockwarr commented Jul 8, 2024

Just to clarify, is this going to be backported into the v0.38.x line?

@melekes
Copy link
Contributor Author
melekes commented Jul 9, 2024

Just to clarify, is this going to be backported into the v0.38.x line?

We are not going to backport this since we consider this a feature, not a bug. We never claimed to support the dash symbol, only [a-zA-Z_.]. For instance, if tomorrow someone comes and tells us they want keys to contain the $ sign, we are going to treat it as a feature too. According to our backporting policy, v0.38 and v0.37 will only backport bug fixes.

@melekes melekes added this pull request to the merge queue Jul 9, 2024
Merged via the queue into main with commit a3ec97e Jul 9, 2024
39 checks passed
@melekes melekes deleted the 3401-dash-event branch July 9, 2024 06:39
@Lockwarr
Copy link
Contributor
Lockwarr commented Jul 9, 2024

Well, in the v0.37 line, querying wasm- events worked fine. I saw there was some sort of refactoring of the way queries are scanned.
if tomorrow someone comes and tells us they want keys to contain the $ sign I disagree with this. I am not trying to get a feature for a new symbol approved. I'm simply migrating our l1 and wanted to keep the functionality from previous versions. If this is not backported in v0.38, we will be blocked until the new release(v1?) as querying events is crucial part for debugging
cc @melekes @hvanz

@melekes
Copy link
Contributor Author
melekes commented Jul 9, 2024

Thanks for clarifying that @Lockwarr. So it appears we were accidentally supporting - symbol (and probably many others) due to the bug? in v0.37 query parser's implementation. I think we can backport this PR, but I'm a little bit afraid of setting a bad precedent (i.e. if - worked in v0.37 and we fixed that, why not allow $, (, ) and all other existing symbols). From the implementation perspective, we actually don't care if a key is $%^&**), but we were trying to keep things as simple as possible.

@melekes
Copy link
Contributor Author
melekes commented Jul 9, 2024

@mergify backport v0.38.x

@melekes
Copy link
Contributor Author
melekes commented Jul 9, 2024

@mergify backport v1.x

@mergify Mergify
Copy link
Contributor
mergify bot commented Jul 9, 2024 •
edited
Loading

backport v0.38.x

✅ Backports have been created

@mergify Mergify
Copy link
Contributor
mergify bot commented Jul 9, 2024 •
edited
Loading

backport v1.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 9, 2024
@melekes @mergify
feat(libs/pubsub): allow dash symbol in event type/attribute (#3456)
e465cab 
Closes #3401

---

#### PR checklist

- [x] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Lockwarr <denislav66@gmail.com>
(cherry picked from commit a3ec97e)

# Conflicts:
#	docs/app-dev/indexing-transactions.md
mergify bot pushed a commit that referenced this pull request Jul 9, 2024
@melekes @mergify
feat(libs/pubsub): allow dash symbol in event type/attribute (#3456)
2ccba79 
Closes #3401

---

#### PR checklist

- [x] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Lockwarr <denislav66@gmail.com>
(cherry picked from commit a3ec97e)

# Conflicts:
#	docs/guides/app-dev/indexing-transactions.md
This was referenced Jul 9, 2024
Merged
Merged
melekes added a commit that referenced this pull request Jul 9, 2024
@mergify @melekes
feat(libs/pubsub): allow dash symbol in event type/attribute (backport …
a1b6c68 
…#3456) (#3461)

Closes #3401 

---

#### PR checklist

- [x] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #3456 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
melekes added a commit that referenced this pull request Jul 9, 2024
@mergify @melekes
feat(libs/pubsub): allow dash symbol in event type/attribute (backport …
a095cb8 
…#3456) (#3462)

Closes #3401 

---

#### PR checklist

- [x] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #3456 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@faddat faddat mentioned this pull request Oct 15, 2024
Closed
3 tasks
745B
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cason cason cason approved these changes

@hvanz hvanz hvanz approved these changes

Assignees

@melekes melekes

Labels
indexer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix querying events with dash(-) in their name
4 participants
@melekes @Lockwarr @cason @hvanz
0