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

Describe identifiers more clearly in help and documentation #128

Merged

Conversation

Bharath-Ganesh
Copy link
Contributor

@Bharath-Ganesh Bharath-Ganesh commented Apr 2, 2023

The documentation for the input step used ID when taking about the id field.
I have refactored all the references of ID to id

LINK: https://issues.jenkins.io/browse/JENKINS-69889

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Copy link
Contributor

@MarkEWaite MarkEWaite 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 pull request and thanks for being involved in the Jenkins project.

I think that the guidance from @jtnord was that the acronym "ID" would be better expressed as the word "identifier" since it could be confused with many different forms of "ID". The page linked by @daniel-beck shows many possible expansions of the acronym "ID".

I don't have a strong objection to the existing changes in this pull request, though I think the changes that I've proposed will better address the concerns expressed by @jtnord and the questions raised by @daniel-beck .

There is a reference to Custom ID that is not covered in this pull request. I would not block the merge of this pull request based on that remaining reference, but I think it would be nice if this pull request or a future pull request also changed that reference. The picture I took looks like this:

id-or-identifier

Again, this pull request is already a step forward. I'm fine if it is merged as is or if it is merged with the changes that I suggested.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
@@ -23,14 +23,14 @@ Input Example with `OK Button Caption`(Optional):
input message: 'input-message', ok: 'OK'

Input Example with `Allowed Submitter`:
User IDs and/or external group names of person or people permitted to respond to the input, separated by ','. Spaces will be trimmed automatically, so "Alice, Bob, Charles" is the same as "Alice,Bob,Charles".
User ids and/or external group names of person or people permitted to respond to the input, separated by ','. Spaces will be trimmed automatically, so "Alice, Bob, Charles" is the same as "Alice,Bob,Charles".
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, I think that many places in the Jenkins documentation and changelogs use the word "username" to refere to the identifier of a user.

Suggested change
User ids and/or external group names of person or people permitted to respond to the input, separated by ','. Spaces will be trimmed automatically, so "Alice, Bob, Charles" is the same as "Alice,Bob,Charles".
Usernames and/or external group names of those permitted to respond to the input, separated by ','. Spaces will be trimmed automatically, so "Alice, Bob, Charles" is the same as "Alice,Bob,Charles".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

README.md Outdated Show resolved Hide resolved
@@ -1,6 +1,6 @@
<p>
Every <code>input</code> step has an unique ID. It is used in the generated URL to proceed or abort.
Every <code>input</code> step has an unique id. It is used in the generated URL to proceed or abort.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Every <code>input</code> step has an unique id. It is used in the generated URL to proceed or abort.
Every <code>input</code> step has an unique identifier. It is used in the generated URL to proceed or abort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Apr 2, 2023

You may also want to rephrase the title of the pull request. Maybe consider:

Describe identifiers more clearly in help and documentation

@Bharath-Ganesh Bharath-Ganesh changed the title ID is an identification document not an identifier #69889 Describe identifiers more clearly in help and documentation Apr 4, 2023
@Bharath-Ganesh Bharath-Ganesh force-pushed the bhganesh/jenkinsci/refactor_ID_to_id branch from 913a8fe to e9d32e1 Compare April 4, 2023 08:03
@Bharath-Ganesh
Copy link
Contributor Author

Thanks @MarkEWaite . Do I need to do anything else to get this merged?

@MarkEWaite
Copy link
Contributor

Thanks @MarkEWaite . Do I need to do anything else to get this merged?

Not as far as I can tell. It will need to wait for a maintainer to review, approve, and merge it.

@jglick jglick merged commit 339683a into jenkinsci:master Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants