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

Fix minor typos #716

Closed
wants to merge 2 commits into from
Closed

Conversation

realsobek
Copy link
Contributor

@realsobek realsobek commented Apr 15, 2017

No description provided.

@realsobek
Copy link
Contributor Author

There are 353 occurrences of 'plugable'. 2 in file names. 351 in code.
I can make the change in a separate branch. But I cannot evaluate the outcome. Hence I would like to have your opinion before jumping to conclusions. Can I go ahead and make the change?

@abbra
Copy link
Contributor

abbra commented Apr 17, 2017

Thanks for this pull request.

There are no tickets associated with these changes.

The changes themselves are controversial. Do not change --forwarder-* to --forward-* because you are dealing with well-known DNS term here, not a simple word.

Please normalize your From: line to be from the same email address. We do not accept something like From: user <user@e5720.Speedport_W_724V_Typ_A_05011603_05_020>

Updates to translations should be done via https://fedora.zanata.org/project/view/freeipa?dswid=2118, see https://fedoraproject.org/wiki/L10N/Translate_on_Zanata for details.

Changes like plugable -> pluggable may be OK in the text when they are part of a normal sentence. However, do not change the code itself and references in the text to those code names. These constitute part of a released plugin API and should not be changed.

@realsobek
Copy link
Contributor Author

Thank you for guiding me. :)

There are no tickets associated with these changes.

In the sense of:
http://www.freeipa.org/page/Contribute/Code#Update_Trac_ticket
I consider my code changes small and omitted the ticket.
Shall I create a ticket to associate the changes with?

Do not change --forwarder-* to --forward-* because you are dealing with well-known DNS term here, not a simple word.

As far as I can see in FreeIPA there is no '--forwarder-policy' option. '--forward-policy' and '--forwarder' options are present.

Please normalize your From: line to be from the same email address.

That is my fault.
It is fixed in my fork now, but the change is not reflected in the pull request. How can I fix this?

What I did to fix the author of the commit on my computer:
$ cd freeipa ; git checkout fix-uk-forwarder-policy
$ git log # output has been shortened manually
commit 2c3db7b
realsobek
add empty lines to be consistent
commit cb4250a
user
use correct option name
commit 703691c
realsobek
fix minor typo in ipa-adtrust-install.1
$ git rebase -i -p 703691c # change file to:
edit cb4250a
keep 2c3db7b

Stopped at cb4250a...

$ git commit --amend --reset-author
[detached HEAD 23a1023] use correct option name
1 file changed, 4 insertions(+), 4 deletions(-)

$ git rebase --continue
Successfully rebased and updated refs/heads/fix-uk-forwarder-policy.

$ git log # output has been shortened manually
commit 7aa5ce3
realsobek
add empty lines to be consistent
commit 23a1023
realsobek
use correct option name
commit 703691c
realsobek
fix minor typo in ipa-adtrust-install.1

$ git commit -a
On branch fix-uk-forwarder-policy
Your branch and 'origin/fix-uk-forwarder-policy' have diverged,
and have 2 and 2 different commits each, respectively.
(use "git pull" to merge the remote branch into yours)
nothing to commit, working directory clean

$ git push
To https://github.com/realsobek/freeipa.git
! [rejected] fix-uk-forwarder-policy -> fix-uk-forwarder-policy (non-fast-forward)
error: failed to push some refs to 'https://github.com/realsobek/freeipa.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

$ git push -f
Counting objects: 8, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (5/5), done.
Writing objects: 100% (8/8), 892 bytes | 0 bytes/s, done.
Total 8 (delta 6), reused 3 (delta 3)
remote: Resolving deltas: 100% (6/6), completed with 3 local objects.
To https://github.com/realsobek/freeipa.git

  • 2c3db7b...7aa5ce3 fix-uk-forwarder-policy -> fix-uk-forwarder-policy (forced update)

Updates to translations should be done via ...

I will do after the '--forwarder-policy' discussion is resolved.

Change like plugable -> pluggable ...

No normal sentence, it is all code.
Maybe it can be changed in the future with the release of a major new version?

@realsobek
Copy link
Contributor Author

realsobek commented Apr 18, 2017

There are no tickets associated with these changes.

On the other hand, if the ticket is required for giving attribution, I will create it.

@stlaz stlaz mentioned this pull request Apr 20, 2017
@realsobek
Copy link
Contributor Author

I cannot get rid of commit cb4250a to normalize From:. Hence I am going to close this PR soon. Afterwards I will create a new branch in my fork, copy all changes (excluding changes to PO files) to it and open a new PR.

@stlaz
Copy link
Contributor

stlaz commented Apr 24, 2017

Please, see what git rebase -i master will do for you, along with git commit --amend --author="Author Name <email@address.com>".

edit: I see a lot of confusion in your commits in this PR, some commits appear multiple times, there are revert and merge commits and that makes this PR simply unmergable. Please note that you don't have to make a new PR, but you can make a new local branch with the changes you want and simply do git push <opt_reponame> newbranch:fix-minor-typos

@realsobek
Copy link
Contributor Author

For some commits the titles were the same, but the contents were different. That was due to my working habit.
The messy git log output was due to my attempts to fix the problems.
git rebase -i HEAD~27 worked wonders to beautify the commits. I can squash the two remaining commits, if required.
git push <opt_reponame> newbranch:fix-minor-typos worked fine too.

</li>
<li>
Enter the name of the domain against which you want to authenticate, for example, <code class="example-domain">.example.com.</code>
Enter the name of the domain against which you want to authenticate, <code class="example-domain">.example.com</code> as an example.
Copy link
Contributor

Choose a reason for hiding this comment

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

This one change I could do without.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested again on CentOS 7 (which runs the FreeIPA server and on which Firefox is started) with Firefox 52.1 (previously 52.0):
With ".example.com." single-sign on (SSO) does not work.
With ".example.com" SSO works.

Removing the dot from within the code tags is the necessary fix.
Moving the dot behind the closing code tag looks good. Then I thought of copy-pasting the text from the website to plain text file. This use case would reintroduce the problem. That is the reason for the rewording.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see what you did there. It's a very valid change, then.

@stlaz
Copy link
Contributor

stlaz commented May 4, 2017

Except for the one change I pointed out, this is all OK with me. The only thing I am not sure is whether we can go changing the doc texts in ipaclient/remote_plugins/2_*/*.py since these are kept for backward compatibility but I hope someone can clear this out for me.
If you could possibly remove the change at the line I noted, I will ACK this as soon as we can be sure about those changes in the remote_plugins/ directory.

@stlaz stlaz self-assigned this May 15, 2017
@@ -80,14 +80,14 @@ def retrieve_netbios_name(api):

def set_and_check_netbios_name(netbios_name, unattended, api):
"""
Depending if trust in already configured or not a given NetBIOS domain
name must be handled differently.
Depending on whether trust is already configured or not a given NetBIOS
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix this sentence to "Depending on whether a trust is already configured or not, the received NetBIOS domain name must be handled differently."

@stlaz
Copy link
Contributor

stlaz commented May 15, 2017

I asked today at a meeting and the ipaclient/remote_plugins/2_*/*.py changes are fine. If you could possibly change the one small issue, we will finally be able tu push this :)

@stlaz
Copy link
Contributor

stlaz commented May 19, 2017

The best we can do now is to push your changes, I will make the addition change :) Thank you for your patches.

@stlaz stlaz added the ack Pull Request approved, can be merged label May 19, 2017
@MartinBasti
Copy link
Contributor

master:

  • bdd88a3 fix spelling mistake; minor rewording
  • a0566ed fix minor spelling mistakes

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label May 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants