-
Notifications
You must be signed in to change notification settings - Fork 8
Add NAMESPACE-file-plugin #1983
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
base: main
Are you sure you want to change the base?
Conversation
| } | ||
|
|
||
| export interface NAMESPACEFormat { | ||
| this: NamespaceInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do not use a keyword here :D
| public readonly version = new SemVer('0.1.0'); | ||
|
|
||
| public applies(file: PathLike): boolean { | ||
| return /^(NAMESPACE|NAMESPACE\.txt)$/i.test(file.toString().split(/[/\\]/).pop() ?? ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return /^(NAMESPACE|NAMESPACE\.txt)$/i.test(file.toString().split(/[/\\]/).pop() ?? ''); | |
| return /^(NAMESPACE(\.txt)?)$/i.test(file.toString().split(/[/\\]/).pop() ?? ''); |
additionally, maybe we outsource:
- the regex to a static member
crymove the "give me filename" thing that we also use for the description plugin into a helper function
| public process(_ctx: FlowrAnalyzerContext, file: FlowrFileProvider<string>): FlowrNamespaceFile { | ||
| const f = FlowrNamespaceFile.from(file, SpecialFileRole.Namespace); | ||
| // already load it here | ||
| f.content(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we do not want this here :P this should be also removed from the description wrapper i guess 🤣
| loadsWithSideEffects: boolean; | ||
| } | ||
|
|
||
| export interface NAMESPACEFormat { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to have this as NamespaceFileFormat (similar to the info)
| * @see {@link parseNAMESPACE} for details on the parsing logic. | ||
| */ | ||
| protected loadContent(): NAMESPACEFormat { | ||
| return parseNAMESPACE(this.wrapped); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly i would like lowercase here
| * | ||
| * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can these be removed?
| * | |
| * |
| loadsWithSideEffects: false, | ||
| }; | ||
| } | ||
| result[pkg].loadsWithSideEffects = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this work? :D - the side effects part is also something that probably has to come from an external source that is unrelated to the namespace plugin but setting it here feels fair
| if(parts.length !== 2) { | ||
| continue; | ||
| } | ||
| const [pkg, _func] = parts; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you do not use _func just [pkg] should be fine
| S3method(as.character,expectation) | ||
| S3method(compare,character) | ||
| export(auto_test) | ||
| export(auto_test_package) | ||
| export(colourise) | ||
| export(context) | ||
| exportClasses(ListReporter) | ||
| exportClasses(MinimalReporter) | ||
| importFrom(methods,setRefClass) | ||
| useDynLib(testthat,duplicate_) | ||
| useDynLib(testthat,reassign_function)`)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice, but for this feature, just because it is important (because we want to mine a looot of packages soon), i would like to have maybe 5-6 more real-world tests if this is ok for you
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file also contains a lot of interesting spacing 😅
| p1.addInfo(undefined, undefined, new Range('<= 2.3')); | ||
| p1.addInfo(undefined, undefined, new Range('>= 1.5')); | ||
| p1.addInfo(undefined, undefined, new Range('<= 2.2.5')); | ||
| p1.addInfo('package', undefined, undefined, new Range('>= 1.3')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again this should be an object with named properties, otherwise this will kill us in the future
Uh oh!
There was an error while loading. Please reload this page.