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.

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

Reader Package Deprecation Implementation(npm) #45

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Reader Package Deprecation Implementation(npm) #45

wants to merge 1 commit into from

Conversation

Ash-KODES
Copy link

@Ash-KODES Ash-KODES commented Mar 12, 2023 •
edited
Loading

FIXES #15
Approach-
Will try to implement all the method that uses the reader package and then refractor and deprecate the reader's methods.
npm implementation for the deprecating package.

puerco
puerco requested changes Mar 13, 2023
View reviewed changes
Copy link
Member

@puerco puerco left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey thanks for the PR @Ash-KODES, if we are changing the module to deprecate the reader, can we do it in a single go? If you want to keep it to smaller PRs it is fine but I want to make sure we strip the module.

Also, two nits, first, please squash your commits to keep them scoped to the change you are introducing.

Plus, please wrap errors 👇

npm/handler.go Show resolved Hide resolved
@puerco
Copy link
Member

puerco commented Mar 13, 2023

(btw we expect some tests to still fail as we are repairing them)

@Ash-KODES
Copy link
Author

is fine but I want to make sure we strip the module.

Ok, So there are two occurrences for the reader package, yarn and npm I can make one PR for yarn and then and PR for Npm.
since,I have made changes in npm rightnow so this PR would be now in context of npm now.
Thanks.

@Ash-KODES Ash-KODES changed the title Reader Package Deprecation Implementation Reader Package Deprecation Implementation(npm) Mar 13, 2023
@Ash-KODES
Copy link
Author

Ash-KODES commented Mar 14, 2023 •
edited
Loading

  • Squashed previous commits into major commit
  • Added error messages
  • npm methods done, reader import removed.

@Ash-KODES Ash-KODES requested a review from puerco March 14, 2023 20:13
@Ash-KODES
Copy link
Author

Ash-KODES commented Mar 15, 2023 •
edited
Loading

@puerco @nishakm Can you please give it a look?

@Ash-KODES
Copy link
Author

@puerco @nishakm Can you please give it a look?

Can someone please review this PR?

@nishakm
Copy link
Contributor

nishakm commented Sep 5, 2023

Hi @Ash-KODES, would it be possible for you to rebase your changes so we can test it?

@Ash-KODES
Copy link
Author

Hi @Ash-KODES, would it be possible for you to rebase your changes so we can test it?

sure @nishakm

@Ash-KODES
Copy link
Author

@puerco @nishakm Please Review, I have rebased it.

@Ash-KODES
Copy link
Author

Hey Guys @nishakm @puerco ,I want to ask few questions regarding these projects,do we have a channel or how can i connect with you guys ?
thanks & Regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@puerco puerco Awaiting requested review from puerco

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate reader package
3 participants
@Ash-KODES @puerco @nishakm