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

Add updating existing open issue with the same title if it exists #71

Merged
merged 25 commits into from
Dec 6, 2020
Merged

Add updating existing open issue with the same title if it exists #71

merged 25 commits into from
Dec 6, 2020

Conversation

PrinsFrank
Copy link
Contributor

Hey @JasonEtco, I tried to implement #63 , but I'm a bit out of my depth here as I'm not familiar with developing actions. How can I test these changes without publishing my fork on the marketplace?

@PrinsFrank PrinsFrank changed the title WIP: Add updating existing open issue with the same title if it exists Add updating existing open issue with the same title if it exists Nov 7, 2020
Copy link
Owner

@JasonEtco JasonEtco left a comment

Choose a reason for hiding this comment

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

This is awesome @PrinsFrank 😱 ❤️

I left a couple of notes for improvements. As for testing, you have two options - either writing unit tests here and seeing that they pass (which we need to do anyway), or using your forked action. It doesn't need to be published to Marketplace; you can just do:

uses: PrinsFrank/create-an-issue@feature-update-existing-issue-when-present

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@PrinsFrank
Copy link
Contributor Author

I think I have everything running correctly, and will continue tomorrow with writing tests.

Copy link
Owner

@JasonEtco JasonEtco left a comment

Choose a reason for hiding this comment

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

This looks great @PrinsFrank ✨ The tests are perfect, the implementation is 🔥. Thank you for all your work on it so far!

I left a couple of very minor comments, the one in the search query is needed to make sure we're finding the right issue titles. Let me know if I can clarify anything! I think after these this should be good to go 🚀

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
tests/index.test.js Outdated Show resolved Hide resolved
tests/index.test.js Outdated Show resolved Hide resolved
tests/index.test.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@PrinsFrank
Copy link
Contributor Author

@JasonEtco, do you have any time to review this?

@Ameausoone
Copy link

Hello, I would be interested by this feature ! (Awesome work by the way).

I could implement #55 when this issue will be merged.

Copy link
Owner

@JasonEtco JasonEtco left a comment

Choose a reason for hiding this comment

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

This looks great to me, sorry it took so long for me to come back to re-review it! Thanks for your work on this, especially the tests and docs 🙌

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.

3 participants