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

Adds 'spo listitem attachment get' command. Closes #5221 #5332

Closed
wants to merge 7 commits into from

Conversation

nanddeepn
Copy link
Contributor

Adds spo listitem attachment get command. Closes #5221

@milanholemans
Copy link
Contributor

Thank you @nanddeepn, we'll try to review it ASAP!

@Jwaegebaert Jwaegebaert self-assigned this Sep 3, 2023
Copy link
Contributor

@Jwaegebaert Jwaegebaert left a comment

Choose a reason for hiding this comment

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

Hey @nanddeepn, would you mind updating your PR to utilize ESM? Here's a breakdown of the new changes:

@Jwaegebaert Jwaegebaert marked this pull request as draft September 3, 2023 16:24
@nanddeepn
Copy link
Contributor Author

Hi @Jwaegebaert

Can you please guide further on below point in spec.ts?
Replace all instances of require with import

@milanholemans
Copy link
Contributor

milanholemans commented Sep 4, 2023

Hi @nanddeepn

Because you cannot use require anymore in ESM, you have to refactor const command: Command = require('./listitem-attachment-get') to import command from './listitem-attachment-get.js';. When in doubt, you can always check another command how it's implemented over there.

@nanddeepn
Copy link
Contributor Author

Thank you @milanholemans

@nanddeepn
Copy link
Contributor Author

Build failed with error:
Error: '/Users/runner/work/_actions/_temp_8e0dd213-3e1a-4223-a95a-b5681a6bf487/5fa5e269-a8ea-4d88-b889-0796cd17e8eb.tar.gz' contains '0' directories

Any idea, anyone?

@nanddeepn
Copy link
Contributor Author

Hi @milanholemans
Can you please help to rerun the test cases?

@nanddeepn nanddeepn marked this pull request as ready for review September 5, 2023 06:44
Copy link
Contributor

@Jwaegebaert Jwaegebaert left a comment

Choose a reason for hiding this comment

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

Hey @nanddeepn, thanks for the changes. Could you also rebase the latest changes from main into this PR? Right now I'm not able to test this PR locally without a rebase from the main branch.

I've also left a few pointers you can take a look at.

@Jwaegebaert Jwaegebaert marked this pull request as draft September 8, 2023 14:16
@nanddeepn
Copy link
Contributor Author

Hi @Jwaegebaert
I had an issue with rebase from main. Raised new PR at #5475
Please review.

@nanddeepn nanddeepn closed this Sep 11, 2023
@nanddeepn nanddeepn deleted the issue-5221 branch September 24, 2023 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New command: spo listitem attachment get
3 participants