Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: ws1 1455 e2e test for get events #95
feat: ws1 1455 e2e test for get events #95
Changes from 2 commits
b3aef10
54fce43
b57718f
8d9a4d0
be87e9b
bc058ad
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe want to update the
fast/events.ts
random event generation to use these functions too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Don't think it should be in this PR though cause it does not have anything to do with the e2e test for fetching events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would just be a minor refactor though? Shouldn't add too much to this PR's scope and the code comes out cleaner versus someone going in afterwards in a separate PR just for reducing code duplication. WDYT?
I don't have a strong opinion though, it's cool to open a ticket for cleanup, maybe tagged as
debt
.Deciding whether to change adjacent code in a PR is more of an art than a science. In PRs with a lot of moving parts, unrelated changes can certainly be a distraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a separate issue for the clean-up. I am planning to move all re-usable functions (not too many) from event.rs to the utils.
Can save us some time in the future :)
https://linear.app/3boxlabs/issue/WS1-1475/clear-some-technical-debt-for-rust-ceramic-and-cermic-tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, ticket looks good. I agree with Mohsin that there is a lot of nuance to refactors. In this case, we're still adding and changing these tests, so I don't think it's a problem to follow up.
My general thinking is this: as long as it doesn't blow up the testing scope, cause breaking changes, make review a lot harder, take way longer, or cause difficult merge conflicts, it's worth doing now. For the review piece, putting those refactoring changes (e.g. moving code across files or renaming) in their own commits can make it easier.
Basically, my preference is to do any refactoring I can when I'm in the code because who knows when I'll be back and have the chance, and there's the overhead of reloading the context into my brain and getting another review etc. It's nice to whittle a little tech debt every PR if possible. I think it's harder to sell "our cycle is tackling tech debt" than "we're doing these features and these tickets are a few extra points so we can take care of things" to leadership.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, on addition it might be better to remove debt on the file being touched in the PR, this might be a more standardized way of removing debt. Since a developer has visibility of the file being worked on, introducing refactoring to another file that is not of interest to the PR might increase scope in review rather than having a predefined scope for the task