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

rename data pkg Stream to Item #3966

Merged
merged 5 commits into from
Aug 11, 2023
Merged

Conversation

ryanfkeepers
Copy link
Contributor

@ryanfkeepers ryanfkeepers commented Aug 4, 2023

A Stream is a continuous transmission of data.
An item is a single structure. Crossing the two
definitions generates confusion.

Primarily code movement/renaming. Though there
is also some reduction/replacement of structs
where we'd made a variety of testable Item implementations
instead of re-using the generic mock.


Does this PR need a docs update or release note?

  • ⛔ No

Type of change

  • 🧹 Tech Debt/Cleanup

Test Plan

  • ⚡ Unit test
  • 💚 E2E

@ryanfkeepers ryanfkeepers added the tech-debt Non-feature, non-bug improvements to the codebase. label Aug 4, 2023
@ryanfkeepers ryanfkeepers self-assigned this Aug 4, 2023
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 4, 2023 17:41 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 4, 2023 17:41 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 4, 2023 17:41 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 4, 2023 17:41 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 4, 2023 17:41 — with GitHub Actions Inactive
@aviator-app
Copy link
Contributor

aviator-app bot commented Aug 4, 2023

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.


See the real-time status of this PR on the Aviator webapp.

@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 7, 2023 20:12 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 7, 2023 20:12 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 7, 2023 20:12 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 7, 2023 20:12 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 7, 2023 20:12 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 7, 2023 20:12 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 7, 2023 20:12 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 7, 2023 20:12 — with GitHub Actions Inactive
Copy link
Contributor

@ashmrtn ashmrtn left a comment

Choose a reason for hiding this comment

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

LGTM, can we wait to merge this until after the corso extension stuff goes in though? It's probably going to create merge conflicts so would be best to let the extensions stuff go in and resolve conflicts here. I think mostly it'll be #3987 that has them but I can't be certain

// ErrNotFound for all Fetch calls.
type NoFetchRestoreCollection struct {
Collection
FetchItemByNamer
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I know this is code movement, but this field should be removed. We're providing a function that gives this functionality, not using an embedded interface

Suggested change
FetchItemByNamer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix that in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ashmrtn please see: #4013

DeletedFlag bool
Err error
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please add a comment describing what Err and ReadErr are for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@pandeyabs
Copy link
Contributor

Thanks @ashmrtn! @ryanfkeepers could you please hold off on this PR while the extension PRs go through?

@ryanfkeepers
Copy link
Contributor Author

LGTM, can we wait to merge this until after the corso extension stuff goes in though? It's probably going to create merge conflicts so would be best to let the extensions stuff go in and resolve conflicts here

Yep! Not a problem.

@@ -0,0 +1,54 @@
package data
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved out of /data/collections to separate interfaces and non-interfaces (the code was getting jumbled in there). Only movement, nothing new/changed.

// ErrNotFound for all Fetch calls.
type NoFetchRestoreCollection struct {
Collection
FetchItemByNamer
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix that in a follow-up PR.

DeletedFlag bool
Err error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

A Stream is a continuous transmission of data.
An item is a single structure.  Crossing the two
definitions generates confusion.
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 10, 2023 18:38 — with GitHub Actions Inactive
@aviator-app aviator-app bot added the blocked Upstream item prevents completion label Aug 10, 2023
@aviator-app
Copy link
Contributor

aviator-app bot commented Aug 10, 2023

This pull request failed to merge: some CI status(es) failed. Remove the blocked label to re-queue.

Failed CI(s): Test-Suite-Trusted

@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 11, 2023 17:07 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 11, 2023 17:07 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers removed the blocked Upstream item prevents completion label Aug 11, 2023
@aviator-app aviator-app bot temporarily deployed to Testing August 11, 2023 17:07 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing August 11, 2023 17:07 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing August 11, 2023 17:08 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing August 11, 2023 17:08 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing August 11, 2023 17:08 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing August 11, 2023 17:08 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing August 11, 2023 17:08 Inactive
@aviator-app aviator-app bot added the blocked Upstream item prevents completion label Aug 11, 2023
@aviator-app
Copy link
Contributor

aviator-app bot commented Aug 11, 2023

This pull request failed to merge: some CI status(es) failed. Remove the blocked label to re-queue.

Failed CI(s): Test-Suite-Trusted

@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 11, 2023 18:53 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 11, 2023 18:53 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 11, 2023 18:53 — with GitHub Actions Inactive
@sonarcloud
Copy link

sonarcloud bot commented Aug 11, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.6% 0.6% Duplication

@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 11, 2023 18:54 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 11, 2023 18:54 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 11, 2023 18:54 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 11, 2023 18:54 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing August 11, 2023 18:54 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers removed the blocked Upstream item prevents completion label Aug 11, 2023
@aviator-app aviator-app bot merged commit 0a94738 into main Aug 11, 2023
22 checks passed
@aviator-app aviator-app bot deleted the unify-colleciton-interfaces branch August 11, 2023 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mergequeue tech-debt Non-feature, non-bug improvements to the codebase.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants