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

[1523] Get supported formats extensions dynamically #1524

Merged
merged 5 commits into from
Dec 14, 2022

Conversation

pedro-mendonca
Copy link
Member

What?

Dynamically generate the supported formats file extensions for file imports.

Why?

If a plugin adds a new file format, dynamically get all the available extensions.

How?

Use the set Formats.

Testing Instructions

Go to the project import dialog and click the button to upload.

Fix #1523

@pedro-mendonca pedro-mendonca marked this pull request as draft November 16, 2022 09:51
@pedro-mendonca pedro-mendonca marked this pull request as ready for review November 16, 2022 10:38
Copy link
Member

@akirk akirk left a comment

Choose a reason for hiding this comment

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

Could we please add a unit test for this?

Copy link
Member

@akirk akirk left a comment

Choose a reason for hiding this comment

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

I think changes are warranted as we’re making assumptions about the file extensions inside a template file and I think it’d be better suited to put this logic inside the GP_Format class. We could introduce a new function like get_acceptable_file_extension()

@pedro-mendonca
Copy link
Member Author

pedro-mendonca commented Nov 30, 2022

We could introduce a new function like get_acceptable_file_extension()

Should I add it to GP_Format?

@akirk
Copy link
Member

akirk commented Nov 30, 2022

Yes, I think it'd be a good place.

@pedro-mendonca
Copy link
Member Author

Yes, I think it'd be a good place.

So you suggest a new get_acceptable_file_extension() that strips any first part of the extension (eg. [jed.]json). Should this method also add the "." to the extensions?

@pedro-mendonca
Copy link
Member Author

pedro-mendonca commented Nov 30, 2022

With the exception of this list of extensions used in this PR where only the last part of the extension counts for the browser to look up, I don't see much of a use for a new method that uses the output of get_file_extensions() and strip any first part of the extension in case there are multiple parts (eg. jed.json), in the GP_Fotmat class.
Also, adding a "." is just for use in this browser lookup situation, considering all that needs to be done afterward, it doesn't make sense to me to add it in a GP_Format output.

A different option would be adding a function to misc.php where all this is done: getting all the extensions, optionally stripping the non-ending part of the extensions, removing duplicates, and adding the ".", a new gp_get_formats_extensions() that would output a simple array of all the formats extensions, with no duplicates.

@pedro-mendonca
Copy link
Member Author

pedro-mendonca commented Nov 30, 2022

@akirk here's an example of what I have in mind:

A function gp_get_format_extensions() which outputs all the formats prefixed with . delimiter, allowing multiple extensions like .jed.json.

gp_get_format_extensions():

array(
  '.xml',
  '.po',
  '.pot',
  '.mo',
  '.resx',
  '.resx.xml',
  '.strings',
  '.properties',
  '.json',
  '.jed.json',
  '.ngx.json',
)

The multiple extensions like .jed.json aren't redundant as it allows the user to select only that option, filtering out files with the single extension .json. Because of this, I think I should remove the preg_replace that strips out the multiple extensions.

Would this make sense to add to misc.php?

@pedro-mendonca pedro-mendonca marked this pull request as draft November 30, 2022 16:51
@pedro-mendonca pedro-mendonca marked this pull request as ready for review December 14, 2022 08:39
Copy link
Member

@akirk akirk left a comment

Choose a reason for hiding this comment

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

misc.php is a reasonable place since other functions also operate on GP::$formats there.

@amieiro amieiro enabled auto-merge (squash) December 14, 2022 08:50
@amieiro amieiro merged commit c4e41e3 into GlotPress:develop Dec 14, 2022
@pedro-mendonca pedro-mendonca deleted the formats branch December 14, 2022 09:15
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.

Dinamically get the supported format types to import
3 participants