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

Introduce separate translations for API documentation #733

Merged
merged 10 commits into from
Jan 22, 2024

Conversation

newstler
Copy link
Contributor

@newstler newstler commented Jan 8, 2024

This PR introduces separate translations for API: api_title and api_description. Those are to be used in OpenAPI documentation without affection of the UI.

  • Adds those fields to Super Scaffolding;
  • Updates translations in core gems;
  • Adds rake task bullet_train:api:create_translations which updates existing locale files, and creates backup translation files in tmp/backup_locales if the original file was updated.

@newstler newstler marked this pull request as draft January 8, 2024 17:14
@newstler newstler marked this pull request as ready for review January 12, 2024 13:49
@newstler newstler marked this pull request as draft January 12, 2024 13:51
@newstler newstler marked this pull request as ready for review January 13, 2024 16:46
@newstler newstler self-assigned this Jan 13, 2024
@newstler newstler changed the title Introduce separate translations for api Introduce separate translations for API documentation Jan 13, 2024
Copy link
Contributor

@jagthedrummer jagthedrummer 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 good to me. Just not sure about the included rake task, and whether we need to document it.

@@ -146,5 +146,103 @@ namespace :bullet_train do
)
end
end

desc "Create `api_title` and `api_description` translations"
task create_translations: :environment do
Copy link
Contributor

Choose a reason for hiding this comment

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

@newstler Is this a task that's intended to be used by end users/developers? If so, we should probably document it somewhere. And maybe mention in the PR description that people should run it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, sure. Where do we usually document such things?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't exactly know. Why and when would people need to use it? Maybe that can help us figure out where to surface it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They only need to run it once, to update all the old translations they have. As there is no documentation for the bt-api gem, probably there should be a post-install massage? I have some thoughts about bt-api README, but right now seems post-install message should work just fine, I'll try to add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest I would even run this task automatically after gem install, do you think this is an acceptable approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a huge fan of the idea of running it automatically. Maybe if it's extremely fast to run. But otherwise it's going to be taking time every time someone runs bundle install whether locally or in CI/CD.

I think a post-install message would be better than automatically running.

Copy link
Contributor Author

@newstler newstler Jan 19, 2024

Choose a reason for hiding this comment

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

Added post-install message in bullet_train-api/bullet_train-api.gemspec.

I will try to create README for bt-api in a separate PR soon, and will mention this (and other) rake tasks there.

@jagthedrummer jagthedrummer merged commit f3eeddd into main Jan 22, 2024
30 checks passed
@jagthedrummer jagthedrummer deleted the feature/separate_translations_for_api branch January 22, 2024 16:42
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.

2 participants