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 active lock reason to issue #2525

Merged
merged 12 commits into from
Aug 5, 2022

Conversation

notauserx
Copy link
Contributor

Added LockReason enum, added activelockreason property to Issue class.

Fixes issue #2248

@notauserx
Copy link
Contributor Author

notauserx commented Aug 4, 2022

I need some feedback on this.
Also, do I add the ActiveLockReason on the PullRequest class as well?

@JonruAlveus
Copy link
Collaborator

Hey so it would be good to see an integration test covering this, in IssuesClientTests in Octokit.Tests.Integration.

If you want to add it to PullRequest as well, that would be great! But if you don't, add it as a new issue so we can track it 👍

@notauserx
Copy link
Contributor Author

Hey @JonruAlveus, thanks for the input. However, I can't seem to run all of integration tests.
Integration tests

Am I missing something?

@JonruAlveus
Copy link
Collaborator

JonruAlveus commented Aug 4, 2022

If you head over to here there's instructions on how to set up test accounts for the integration tests to run in. You don't need all of the settings, but try to use a separate github account to your own as you could easily get yourself locked out of github for exceeding api rate limits 😄

I believe you need the first two as a minimum: OCTOKIT_GITHUBUSERNAME and OCTOKIT_GITHUBPASSWORD, which are set as environment variables on your machine.

@notauserx
Copy link
Contributor Author

Thanks @JonruAlveus, I'll give it a go. I have updated the PR with a sample test. Will update when I can set up the test.
I have changed the signature of the lock method on IIssueClient for allowing to set the lock reason. I've made the LockReason parameter optional so I don't have to change all the references.
Feel free to review my changes.

@notauserx
Copy link
Contributor Author

Added CanAccessActiveLockReason on IssuesClientTest and the test is passing.
However, getting "warning: CRLF will be replaced by LF in Octokit/Clients/IssuesClient.cs."
What is the default line ending for the project?

@nickfloyd
Copy link
Contributor

Hey @notauserx EOL in this repo should be lf - you can run git ls-files --eol to validate this in this and other projects in the future if you'd like.

@notauserx
Copy link
Contributor Author

@nickfloyd line endings seems to be correct for the files I modified.

@JonruAlveus
Copy link
Collaborator

Judging by the gitattributes documentation (effects section) (and I’m not an expert on this at all), when you got add or got commit, the line endings are set according to the file. So I guess you could change the line endings and they will just be sorted out when you add/commit.

@nickfloyd
Copy link
Contributor

Judging by the gitattributes documentation (effects section) (and I’m not an expert on this at all), when you got add or got commit, the line endings are set according to the file. So I guess you could change the line endings and they will just be sorted out when you add/commit.

For sure... for .cs files as defined in our .gitattributes file it's LF.

I used to config my local global, just in case the repo I am working in does not have .gitatributes to be - core.autocrlf true on windows and core.autocrlf input on mac that way my changes end up being the more common LF even if the repo owner doesn't have one set. I haven't encountered those issues in years, so I generally leave it as is.

All that to say, for *.cs files and all other files, we should follow what is defined in .gitattribtues for consistency's sake. If that's not working as it should, we need to look at that and understand why.

Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

Just a quick change around the enum value name - looks like a typo. This is looking good, let me know when you take it out of WIP / Draft, and we can get this merged in.

Octokit/Models/Common/LockReason.cs Outdated Show resolved Hide resolved
@notauserx notauserx changed the title [wip] add active lock reason to issue add active lock reason to issue Aug 5, 2022
@notauserx
Copy link
Contributor Author

@nickfloyd thanks for the review and the catch. I've committed your suggestion and removed wip from the name.
Please merge if the PR looks good.

Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

Thanks for the changes ❤️

@nickfloyd nickfloyd merged commit 536e5c3 into octokit:main Aug 5, 2022
@nickfloyd
Copy link
Contributor

release_notes: Adds active lock reason to issue

@notauserx notauserx deleted the add-active-lock-reason branch August 6, 2022 01:46
@nickfloyd nickfloyd added Type: Feature New feature or request and removed category: feature labels Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants