ilib-lint: Add the ability to filter errors from xliff files #62
Conversation
🦋 Changeset detectedLatest commit: 3ee822f The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ehoogerbeets
force-pushed
the
linterFilterErrors
branch
from
ee16dda to
bcac20f
Compare
| */ | ||
| write() { | ||
| if (this.filePath && this.isDirty()) { | ||
| const dir = path.dirname(this.filePath); | ||
| makeDirs(dir); |
There was a problem hiding this comment.
tentative: either fix doc for write, or I'd prefer to get rid of this completely - IMO it does not make sense for why would a file write operation to create directories in the first place
There was a problem hiding this comment.
Also even if we keep it, no need to roll your own recursion:
| makeDirs(dir); | |
| fs.mkdirSync(path.dirname(this.filePath), {recursive: true}); |
There was a problem hiding this comment.
If the config file says to put the output in directory x/y/z and it doesn't exist, it should create it. The author of the config file intended that directory to be there.
There was a problem hiding this comment.
Docs fix needed
The path to the file must exist first.
| * @param {Array.<Result>|undefined} results the results of the rules that were applied earlier | ||
| * in the pipeline, or undefined if there are no results or if the rules have not been | ||
| * applied yet |
There was a problem hiding this comment.
nit: undefined is redundant with empty array
There was a problem hiding this comment.
note/question: why does the transformer need to know about past results?
nit: description for results is confusing, I'd just say results of linting this @link representation
Jest Code CoverageCode Coverage for changed files • (0%)
|
wadimw
force-pushed
the
linterFilterErrors
branch
2 times, most recently
from
cef2dd3 to
7208faf
Compare
- not used yet
- filters out Resource instances that have error Results after linting
- the file stats are already encoded in the intermediate representation
- describe the new command-line parameters for writing files out, applying auto fixes, and overwriting source file - add description to the configuration to help people understand how the configuration works - update configuration to allow for transformers and serializers to be defined in the file type
- no code change so far
- needs some unit tests
Will need some more work here...
- throws exceptions now if you have a plugin for a different representation - more work on the serializer at the end (not working yet) - updated copyright years for changed files to 2025
- ilib convention is to put tests in a top-level test directory, not interspersed throughout the source code in many different __test__ directories
- SourceFile can now make the intermediate directories before writing out the file - Added unit tests for SourceFile.write
- still needs more testing
ehoogerbeets
force-pushed
the
linterFilterErrors
branch
from
7208faf to
3ee822f
Compare
|
Ready for re-review |
| sourceFile: jest.fn(() => "sourceFile") | ||
| }); | ||
|
|
||
| const transformed = eft.transform(representation, []); |
There was a problem hiding this comment.
What about the case when results is undefined?
If I understand correctly, this is possible, as per the ilib-lint docs:
Rules interpret the intermediate representation of a file produced by a Parser and produce a single Result instance, an array of Result instances, one for each problem found, or undefined if there are no problems found.
| } | ||
| const resources = ir.getRepresentation(); | ||
| const idsToExclude = results.filter(result => result.id && result.severity === 'error').map(result => result.id); | ||
| const filteredResources = resources.filter(resource => !idsToExclude.includes(resource.getKey())); |
There was a problem hiding this comment.
Please add a test for filtering out. This is one of the crucial aspects of the mechanics, and it is essential to test it explicitly so that errors can be identified if the filter logic is messed up.
| - getFormatters - return an array of classes that inherit from the Formatter class | ||
| - getTransformers - return an array of classes that inherit from the Transformer class | ||
| - getSerializers - return an array of classes that inherit from the Serializer class |
There was a problem hiding this comment.
getFixers
getFormatters
getTransformers
👇
Does the order matter? If so, what's the correct order?
Should we add information about the ordering here?
| * by type | ||
| * @private | ||
| */ | ||
| byType = {}; |
There was a problem hiding this comment.
Why do we need three caches that store the same information? Since we already have serializerCache, we can retrieve the serializer's name and description by calling serializer.getName() and serializer.getDescription() anytime.
Having only one cache simplifies the system, reduces redundancy, and makes it easier to maintain, as we only need to manage a single source of truth for the data.
| */ | ||
| add(serializers) { | ||
| if (!serializers || !Array.isArray(serializers)) return; | ||
| for (const ser of serializers) { |
There was a problem hiding this comment.
This could be simplified if you used only serializerCache for caching serializers (and their details).
| * | ||
| * @returns {Object} the serializer names and descriptions | ||
| */ | ||
| getDescriptions() { |
There was a problem hiding this comment.
By having only one cache, you could use it here like this (pseudocode):
getDescriptions() {
return this.serializerCache.map(serializer => serializer.getDescription())
}
No description provided.