Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Externalize strings from Extension Manager view item template#3483

Merged
jasonsanjose merged 3 commits into
masterfrom
nj/extmgr-strings
Apr 19, 2013
Merged

Externalize strings from Extension Manager view item template#3483
jasonsanjose merged 3 commits into
masterfrom
nj/extmgr-strings

Conversation

@njx
Copy link
Copy Markdown

@njx njx commented Apr 19, 2013

Note that the template references the strings as {{Strings.STRING_KEY}} instead of just {{STRING_KEY}}. This is because each item is rendered separately, so it seemed worth it to avoid copying all the strings into the context object for each item, as opposed to just pointing to the entire Strings object in a subfield of the context.

@njx
Copy link
Copy Markdown
Author

njx commented Apr 19, 2013

Also, note that I changed "by" and "on" to "Author:" and "Date:". I figured that the new design might separate these various fields out, so having them be prepositions wasn't necessarily going to make sense; also, due to word order differences in foreign languages, it was risky to rely on prepositions to express these relationships (especially since they aren't all coming out of a master string format that can reorder things).

@ghost ghost assigned jasonsanjose Apr 19, 2013
@jasonsanjose
Copy link
Copy Markdown
Member

Assigned to me

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would we want to keep our options open by including "by" in the translation?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I don't think it's really necessary...it's probably going to be pretty obvious from context that these are the author/date even if we don't use any labels at all.

@jasonsanjose
Copy link
Copy Markdown
Member

Mostly minor comments. Could probably merge as-is, but I'll wait until tomorrow in case I don't hear from @njx.

Narciso Jaramillo added 2 commits April 18, 2013 23:58
@njx
Copy link
Copy Markdown
Author

njx commented Apr 19, 2013

Fixed the version checking to indicate whether a newer or older version of Brackets is needed. (It wasn't completely straightforward because semver doesn't directly tell you whether an unsatisfied range is newer or older than the version you have, so I had to parse the range in order to figure that out.)

While testing this I also noticed that I wasn't actually properly forcing the Extension Manager to retrieve the registry every time you open the dialog (which we want to do)--both because I wasn't actually passing the parameter to tell it to do so, and because passing {cache: true} to $.getJSON() doesn't actually work (its second parameter isn't actually an options param), so I snuck in a fix for that as well. Fixing this required me to fix the unit tests, which were actually assuming that the view creation didn't force a new fetch of the registry (and was therefore expecting it to be synchronous).

@njx
Copy link
Copy Markdown
Author

njx commented Apr 19, 2013

@dangoor - you might want to take a look at c465213 to see if the way I'm dealing with semver to determine whether we need an older or newer version of Brackets makes sense here.

@njx
Copy link
Copy Markdown
Author

njx commented Apr 19, 2013

Oh, I also changed "Brackets" to {APP_NAME} in the strings in c465213 as well. The caching fixes are in 0f11a33. Ready for re-review.

@dangoor
Copy link
Copy Markdown
Contributor

dangoor commented Apr 19, 2013

I'm a little conflicted about c465213 for a couple of reasons.

  1. semver (the module, not the standard) supports another version requirement style: "0.20.x".
  2. it also supports more complex requirements: ">0.20.0 <0.23.0"

In general, I think that doing any parsing of version requirements outside of the semver module is asking for trouble.

The other thing I thought of with respect to this change is that we have the ability to actually offer earlier versions of an extension that may be compatible. If you look at the registry data, you'll see things like this:

"versions":[{"version":"1.0.2","published":"2013-04-10T18:21:27.058Z","brackets":">0.20.0"}]

For each version of an extension, we keep track of Brackets compatibility. So, it's possible that the most recent version is not compatible, but the user could install the previous version. That is a feature that can be added later, though. I just mention it because checking entry.versions[entry.version.length-1].brackets is a better way to go than entry.metadata.engines.brackets because it will hint at the future capability and is also a clean place for us to tweak things in the registry if we detect an incompatibility.

All of that said, I think the code here covers the common cases and won't blow up in the uncommon ones. I think the UX of hinting that a newer version of Brackets or an update to the extension is needed is better than just saying "It doesn't work" and is probably worthwhile.

For the purposes of string freeze, I think it's fine to take this code. But, I think that we should ultimately:

  1. use entry.versions[entry.version.length-1].brackets
  2. get something into semver that does the comparison we want (a version of satisfies that hints at the problem when it doesn't satisfy, or maybe there's some way to use maxSatisfying?)

@njx
Copy link
Copy Markdown
Author

njx commented Apr 19, 2013

@dangoor--agreed on both points. Let's merge this as-is today--could you file a bug for the other issues to track them? I think we should fix the first one this sprint--not as sure about the semver change.

@jasonsanjose
Copy link
Copy Markdown
Member

Looks good. Merging.

jasonsanjose added a commit that referenced this pull request Apr 19, 2013
Externalize strings from Extension Manager view item template
@jasonsanjose jasonsanjose merged commit 118f9eb into master Apr 19, 2013
@jasonsanjose jasonsanjose deleted the nj/extmgr-strings branch April 19, 2013 22:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants