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

Remove current localization mechanism #325

Closed
michaelcfanning opened this issue Feb 20, 2019 · 5 comments
Closed

Remove current localization mechanism #325

michaelcfanning opened this issue Feb 20, 2019 · 5 comments
Labels
2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. design-approved The TC approved the design and I can write the change draft design-improvement e-ballot e-ballot-3 impact-breaks-consumers impact-breaks-producers merged Changes merged into provisional draft. p1 Priority 1 issue to close resolved-fixed tc-32

Comments

@michaelcfanning
Copy link
Contributor

No description provided.

@michaelcfanning
Copy link
Contributor Author

Verify with GrammaTech that they don't intend to use the feature, otherwise remove.

@michaelcfanning
Copy link
Contributor Author

michaelcfanning commented Feb 23, 2019

As a consequence of this, we no longer require tool.name to be a message (as it can no longer be localized). Instead it can revert to being a string.

Also, reportingDescriptor.name can revert to being a string.
And exception.message will revert to being a string.
@lgolding

@ghost
Copy link

ghost commented Feb 27, 2019

Paul confirmed that Grammatech isn't using this feature. In an email, I gave four reasons for removing it:

  1. It’s not exercised. I’m not aware of any SARIF-producing tool that offers multiple language output. (If there were one, I’ll bet it’s author would have complained about #4 below!)
  2. Michael has always objected that the current design imposes constraints on how tools store arrange their resource files on the file system or on the web. I’ve argued back that this is necessary to allow tools to locate resources for other languages, but that doesn’t make it a good design.
  3. It’s not useful, as I argued in the email “tool.language considered unnecessary.”
  4. It’s badly designed. It’s always bothered me – and I should have raised this earlier; it’s just something I swept under my mental rug – that the contents of run.resources are a mix a localizable strings and on-localizable metadata. That is, the rule object (as of Provide full metadata objects for notifications #311, the reportingDescriptor object) includes messageStrings and richMessageStrings (localizable) and configuration (level, rank, and parameters: non-localizable). So if you duplicate the resources into multiple languages, you have to keep the rule metadata in sync across all of them.

UPDATE We now have a customer at MS who wants to localize their SARIF files, but @michaelcfanning, @kupsch, and I agree that the current design will not serve, mostly due to reason #4. This proposal removes the current, inadequate design. We are working on a replacement, but it's not ready, so it is not on the e-ballot.

@michaelcfanning
Copy link
Contributor Author

michaelcfanning commented Feb 27, 2019

EBALLOT PROPOSAL: remove existing localization mechanism. See above for rationale. Because we no longer utilize the message.messageId property to drive localization scenarios, certain SARIF properties can be retyped as multiformatMessageString instances.

API IMPACT

  • In the tool object:
    • Remove the language property.
  • In the reportingDescriptor object:
    • Change the type of the name property from message object to string.
    • Change the type of the shortDescription property from message object to multiformatMessageString object.
    • Change the type of the fullDescription property from message object to multiformatMessageString object.
  • In the exception object:
    • Change the type of the message property from message object back to string (as once it was).

NOTES
The TC is developing a new proposal for localization that may replace the current mechanism that we propose to remove (due to its obvious flaws).

@michaelcfanning michaelcfanning added 2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. e-ballot labels Feb 27, 2019
@michaelcfanning michaelcfanning changed the title Consider removing localization mechanism and tool.language Remove current localization mechanism Feb 27, 2019
michaelcfanning pushed a commit to microsoft/sarif-sdk that referenced this issue Mar 6, 2019
@ghost ghost added the design-approved The TC approved the design and I can write the change draft label Mar 13, 2019
@ghost ghost added the e-ballot-3 label Mar 19, 2019
ghost pushed a commit that referenced this issue Mar 22, 2019
ghost pushed a commit that referenced this issue Mar 23, 2019
@ghost ghost self-assigned this Mar 23, 2019
@ghost ghost added impact-breaks-consumers impact-breaks-producers design-improvement change-draft-available p1 Priority 1 issue to close merged Changes merged into provisional draft. labels Mar 23, 2019
@ghost ghost added the resolved-fixed label Apr 6, 2019
@ghost
Copy link

ghost commented Apr 6, 2019

Approved in e-ballot-3.

@ghost ghost closed this as completed Apr 6, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. design-approved The TC approved the design and I can write the change draft design-improvement e-ballot e-ballot-3 impact-breaks-consumers impact-breaks-producers merged Changes merged into provisional draft. p1 Priority 1 issue to close resolved-fixed tc-32
Projects
None yet
Development

No branches or pull requests

1 participant