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

Fixed consistency in language strings #6414

Closed
wants to merge 11 commits into from

Conversation

tangix
Copy link
Contributor

@tangix tangix commented Aug 23, 2022

Description
Referencing #6341
Updated strings in Language/en to improve consistency.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@tangix
Copy link
Contributor Author

tangix commented Aug 23, 2022

Will work on the failed tests once I understand how to run the database tests locally...

@kenjis kenjis added the breaking change Pull requests that may break existing functionalities label Aug 23, 2022
@kenjis
Copy link
Member

kenjis commented Aug 23, 2022

I think this is not a bug fix, so it is better to go to 4.3 branch.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#if-you-sent-to-the-wrong-branch

@kenjis
Copy link
Member

kenjis commented Aug 23, 2022

Will work on the failed tests once I understand how to run the database tests locally...

The database tests work by default, because the default config is to use SQLite3.

@MGatner
Copy link
Member

MGatner commented Aug 23, 2022

@tangix The failures all appear to be expected language string changes. If I were you I would pull up the Actions log for one of the tests (I.e. https://github.com/codeigniter4/CodeIgniter4/runs/7966690208?check_suite_focus=true) and use that to track down the strings that need changing.

Thanks for taking this on! Seeing that we previously had strings like this makes me even more grateful:

Invalid "background" color: "Background".

@MGatner
Copy link
Member

MGatner commented Aug 23, 2022

@kenjis I maintain that this is not a breaking change. We made the decision a while ago (#4068) that the content of language strings is not covered. If developers are relying on language string output they should be checking it against the function call, not the content.

@kenjis kenjis removed the breaking change Pull requests that may break existing functionalities label Aug 23, 2022
@kenjis
Copy link
Member

kenjis commented Aug 23, 2022

@MGatner I did not know it.

It is unreasonable to require users to read and remember all past changelogs.
We need the docs about backwards compatibility.

@MGatner
Copy link
Member

MGatner commented Aug 23, 2022

Agreed. We had a contributor comment to that effect, not sure why it was ignored: #4068 (review)

@tangix
Copy link
Contributor Author

tangix commented Aug 24, 2022

I think this is not a bug fix, so it is better to go to 4.3 branch. See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#if-you-sent-to-the-wrong-branch

@kenjis
Let me know if I should create a new PR for the 4.3 branch.

@kenjis
Copy link
Member

kenjis commented Aug 24, 2022

@MGatner I sent #6418

@kenjis
Copy link
Member

kenjis commented Aug 24, 2022

@tangix As MGatner says this is not a breaking change. So we can merge into develop.
But I'm not sure which branch (develop or 4.3) is better.

@tangix
Copy link
Contributor Author

tangix commented Aug 24, 2022

@kenjis OK, I’ll sit on the sideline watching you sort it out.
Let me know what needs to be done if anything.

@kenjis
Copy link
Member

kenjis commented Aug 24, 2022

@MGatner This is not a breaking change, but the changes are many.
These changes may affect the translations.
So I think it is better to go to 4.3 branch.
What do you think?

@MGatner
Copy link
Member

MGatner commented Aug 24, 2022

Oh yes that's absolutely fine! I was referring to the breaking change label, not the branch. This should be a pretty easy one to switch over. Hopefully 4.3 is coming out soon anyways

@kenjis
Copy link
Member

kenjis commented Aug 25, 2022

@kenjis
Copy link
Member

kenjis commented Aug 25, 2022

@tangix One more thing.
Could you add a short description in changelog Changes section:
https://github.com/codeigniter4/CodeIgniter4/blob/4.3/user_guide_src/source/changelogs/v4.3.0.rst#changes

Something like this is sufficient:

Updated strings in Language/en to improve consistency.

@tangix
Copy link
Contributor Author

tangix commented Aug 25, 2022

Hmmmm, not a github-ninja like you guys but I tried to follow the instructions.
Problem is, I cannot see the 4.3 branch in my fork of the repo, even after syncing. Is there a way to solve this before I give up and recreate my fork?
image

@kenjis
Copy link
Member

kenjis commented Aug 25, 2022

$ git remote add upstream git@github.com:codeigniter4/CodeIgniter4.git
$ git fetch upstream
$ git switch 4.3
$ git push origin 4.3

@tangix tangix changed the base branch from develop to 4.3 August 25, 2022 07:21
@tangix
Copy link
Contributor Author

tangix commented Aug 25, 2022

$ git remote add upstream git@github.com:codeigniter4/CodeIgniter4.git
$ git fetch upstream
$ git switch 4.3
$ git push origin 4.3

OK.... thanks for all the help!
I think all looks good but I leave that for you to go through and verify.

@kenjis kenjis added the 4.3 label Aug 25, 2022
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -31,9 +31,9 @@
'commandType' => 'Command type',
'databaseGroup' => 'Database group',
'fileCreate' => 'File created: {0}',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'fileCreate' => 'File created: {0}',
'fileCreate' => 'File created: "{0}"',

@kenjis
Copy link
Member

kenjis commented Aug 25, 2022

@kenjis kenjis added the stale Pull requests with conflicts label Aug 25, 2022
@tangix
Copy link
Contributor Author

tangix commented Aug 25, 2022

Hmmmm.... don't know if I managed to rebase, in GitKraken it looks like I didn't succeed.
I'm willing to scrap this PR, re-fork my repo and recreate the PR from a fresh copy so I know I'm not making any mistakes.

@kenjis
Copy link
Member

kenjis commented Aug 25, 2022

Obviously you ran merge not rebase, because you added a merge commit.
rebase never adds a commit.

But no problem. All you need to do is still git rebase.

$ git fetch upstream
$ git rebase upstream/4.3

...Resolve conflict...

$ git add -u
$ git rebase --continue

Rebase is complete.

And force push:

$ git push --force-with-lease origin lang-consistency-fixes

@tangix
Copy link
Contributor Author

tangix commented Aug 26, 2022

@kenjis I really appreciate your efforts but seems like I am a complete idiot when it comes to using github (we are using git on a daily bases for multiple projects but rebase is something I have never needed to do).
Don't know if it's GitKraken or something else causing these issues.

As said before - I am perfectly fine re-forking and doing all again. This might save us both a lot of time and efforts.

msa@verdandi CodeIgniter4 % git fetch upstream
msa@verdandi CodeIgniter4 % git rebase upstream/4.3
Auto-merging user_guide_src/source/changelogs/v4.3.0.rst
CONFLICT (content): Merge conflict in user_guide_src/source/changelogs/v4.3.0.rst
error: could not apply 7c9bf6613... Added info to changelog
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 7c9bf6613... Added info to changelog
msa@verdandi CodeIgniter4 % git rebase --continue
[detached HEAD d8e098298] Added info to changelog
 1 file changed, 1 insertion(+)
dropping 9a7039387d6a67bc2173eb3028d3195502165607 Fixed additional tests that failed. -- patch contents already upstream
dropping 8f3cd24c10d090e932eef4181232a2d27189369b Updated failed tests due to language strings changed. -- patch contents already upstream
dropping 5be13c5d8f160c21d00b6ce7eda66a8e644e1ea5 Merged from old branch -- patch contents already upstream
Auto-merging user_guide_src/source/changelogs/v4.3.0.rst
CONFLICT (content): Merge conflict in user_guide_src/source/changelogs/v4.3.0.rst
error: could not apply c63c591b1... Added info to changelog
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply c63c591b1... Added info to changelog
msa@verdandi CodeIgniter4 % git add -u
msa@verdandi CodeIgniter4 % git rebase --continue
[detached HEAD 63f4dcf74] Added info to changelog
 1 file changed, 3 insertions(+)
Successfully rebased and updated refs/heads/lang-consistency-fixes.
msa@verdandi CodeIgniter4 % git push
To https://github.com/tangix/CodeIgniter4.git
 ! [rejected]            lang-consistency-fixes -> lang-consistency-fixes (non-fast-forward)
error: failed to push some refs to 'https://github.com/tangix/CodeIgniter4.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.
msa@verdandi CodeIgniter4 % git pull
hint: Pulling without specifying how to reconcile divergent branches is
hint: discouraged. You can squelch this message by running one of the following
hint: commands sometime before your next pull:
hint:
hint:   git config pull.rebase false  # merge (the default strategy)
hint:   git config pull.rebase true   # rebase
hint:   git config pull.ff only       # fast-forward only
hint:
hint: You can replace "git config" with "git config --global" to set a default
hint: preference for all repositories. You can also pass --rebase, --no-rebase,
hint: or --ff-only on the command line to override the configured default per
hint: invocation.
Auto-merging user_guide_src/source/changelogs/v4.3.0.rst
CONFLICT (content): Merge conflict in user_guide_src/source/changelogs/v4.3.0.rst
Automatic merge failed; fix conflicts and then commit the result.

@kenjis
Copy link
Member

kenjis commented Aug 26, 2022

No problem. If you don't use rebase, you will be in trouble with open source contributions like this.
Because the upstream development is always progressing, and conflicts may happen anywhere.
And it seems you are getting close to finish the rebase.

rebase modifies previous commits. So it is a bit dangerous operation, but your PR branch is yours.
If you break it completely, no harm in the upstream repository.

After running git rebase upstream/4.3, you need to resolve the conflict.
After resolving the conflict, you will run git add -u and git rebase --continue.
It seems you did them.

rebase modifies previous commits, so normal git push won't work.
You need to force push: git push --force-with-lease origin lang-consistency-fixes

And I recommend you don't use git pull. It is harmful, and is not needed here.

@tangix tangix closed this Aug 26, 2022
@tangix tangix mentioned this pull request Sep 2, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 stale Pull requests with conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants