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 command to scan for new translation #5168

Open
wants to merge 39 commits into
base: develop
Choose a base branch
from

Conversation

sdassow
Copy link
Contributor

@sdassow sdassow commented Sep 27, 2024

Description:

Adds a translate command that scans for new translation and updates the translations file.

No new external modules are imported.
The resulting size increase for the fyne command binary is 830kb on this version of macOS:

$ ls -al fyne*
-rwxr-xr-x  1 simon  staff  17764240 Nov  2 13:48 fyne
-rwxr-xr-x  1 simon  staff  16913632 Nov  2 13:47 fyne.develop

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Where applicable:

  • Public APIs match existing style and have Since: line.
  • No breaking changes (new command).
  • Check for binary size increases when importing new modules.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Can you please factor code out of the 'Translate' method? 120 lines seems very long for something that could be broken down. That would also facilitate adding a test :).

Lastly do note to check the boxes on your PR template so we can see where it's at

@coveralls
Copy link

coveralls commented Sep 30, 2024

Coverage Status

coverage: 59.942% (-0.02%) from 59.961%
when pulling 10d57ee on sdassow:add-translate-command
into de361f6 on fyne-io:develop.

@andydotxyz
Copy link
Member

Just FYI we have now update the CI version requirements so you'll need to rebase on latest develop to get a full build to pass

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Found a few logic glitches which I commented inline.

However the biggest comment would be that we avoid code comments unless really required. More often than not an inline comment shows a lack of clarity in the code.

cmd/fyne/internal/commands/translate.go Outdated Show resolved Hide resolved
cmd/fyne/internal/commands/translate.go Outdated Show resolved Hide resolved
cmd/fyne/internal/commands/translate.go Outdated Show resolved Hide resolved
cmd/fyne/internal/commands/translate.go Show resolved Hide resolved
cmd/fyne/internal/commands/translate.go Outdated Show resolved Hide resolved
cmd/fyne/internal/commands/translate.go Outdated Show resolved Hide resolved
@sdassow sdassow marked this pull request as draft October 15, 2024 12:27
@sdassow sdassow changed the title WIP: command to scan for new translation Add command to scan for new translation Oct 15, 2024
@sdassow sdassow self-assigned this Oct 15, 2024
@sdassow sdassow marked this pull request as ready for review October 15, 2024 13:44
Copy link
Member

@andydotxyz andydotxyz 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 pretty epic. Just continuing a comment from before.

@andydotxyz
Copy link
Member

Those Windows errors seem to indicate you haven't closed a file before trying to remove it.

@sdassow
Copy link
Contributor Author

sdassow commented Oct 18, 2024

Those Windows errors seem to indicate you haven't closed a file before trying to remove it.

Sort of like that. The problem was doing chdir into the temp directory in the test as it prevents removal.

@sdassow sdassow requested a review from toaster October 18, 2024 14:24
@sdassow
Copy link
Contributor Author

sdassow commented Oct 23, 2024

This looks pretty epic. Just continuing a comment from before.

Did I miss the comment or was it addressed? I'm not sure at the moment.

@andydotxyz
Copy link
Member

This looks pretty epic. Just continuing a comment from before.

Did I miss the comment or was it addressed? I'm not sure at the moment.

Unfortunately force-pushing after a review often kills comments - GitHub has an habit of losing them so we ask folk not to force after first review. It all looks good though thanks - I will run some tests but pretty excited to see this added!

@andydotxyz
Copy link
Member

Thanks so much, I tried this out but I think it doesn't create the folder if it doesn't exist.

[sdassow-add-translate-command] ><> go run ../fyne translate *.go   
open translations/en.json-182321854: no such file or directory
exit status 1

My first attempt was to pass a directory but it didn't work - should it? :)

Last comment. Our translations were 2 spaces, weblate default is 4 spaces and this is tabs.
It would be good to get this and our internal files consistent at least - so we should agree on the approach.

@sdassow
Copy link
Contributor Author

sdassow commented Oct 26, 2024

Thanks so much, I tried this out but I think it doesn't create the folder if it doesn't exist.

[sdassow-add-translate-command] ><> go run ../fyne translate *.go   
open translations/en.json-182321854: no such file or directory
exit status 1

That's not a nice experience. Changed it.
The first argument is now mandatory instead and specifies the file to use to avoid any confusion.
When the first argument is missing it shows this:

Missing argument: translationsFile
usage: fyne translate [command options] translationsFile [source ...]

My first attempt was to pass a directory but it didn't work - should it? :)

Absolutely, and it does now :)

Last comment. Our translations were 2 spaces, weblate default is 4 spaces and this is tabs. It would be good to get this and our internal files consistent at least - so we should agree on the approach.

Since we use weblate I've changed it to 4 spaces.

@sdassow
Copy link
Contributor Author

sdassow commented Oct 27, 2024

Additionally it's now checking the first argument:

$ ./fyne translate
Missing argument: translationsFile
usage: fyne translate [command options] translationsFile [source ...]
$ ./fyne translate *.go
Need .json extension for translationsFile
usage: fyne translate [command options] translationsFile [source ...]
$

@sdassow
Copy link
Contributor Author

sdassow commented Oct 29, 2024

Additionally it's now checking the first argument:

$ ./fyne translate
Missing argument: translationsFile
usage: fyne translate [command options] translationsFile [source ...]
$ ./fyne translate *.go
Need .json extension for translationsFile
usage: fyne translate [command options] translationsFile [source ...]
$

Why make the translations file a mandatory argument you ask?

  • works regardless of the current directory or sub-directories
  • makes no assumptions about the default language, leaving the decision to the developer

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