-
Notifications
You must be signed in to change notification settings - Fork 10
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
src/project/plugins/file-plugins/flowr-analyzer-namespace-file-plugin.ts
Outdated
Show resolved
Hide resolved
src/project/plugins/file-plugins/flowr-analyzer-namespace-file-plugin.ts
Outdated
Show resolved
Hide resolved
| 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 is still open!
…space-file-plugin
# Conflicts: # src/project/context/flowr-analyzer-context.ts # src/queries/catalog/dependencies-query/dependencies-query-executor.ts # src/queries/catalog/dependencies-query/dependencies-query-format.ts
| */ | ||
| readonly config: FlowrConfigOptions; | ||
|
|
||
| readonly functions: ReadOnlyFlowrAnalyzerFunctionsContext; |
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 add a doc comment
| this.files = new FlowrAnalyzerFilesContext(loadingOrder, (plugins.get(PluginType.ProjectDiscovery) ?? []) as FlowrAnalyzerProjectDiscoveryPlugin[], | ||
| (plugins.get(PluginType.FileLoad) ?? []) as FlowrAnalyzerFilePlugin[]); | ||
| this.deps = new FlowrAnalyzerDependenciesContext(this, (plugins.get(PluginType.DependencyIdentification) ?? []) as FlowrAnalyzerPackageVersionsPlugin[]); | ||
| this.functions = new FlowrAnalyzerFunctionsContext(this, (plugins.get(PluginType.DependencyIdentification) ?? []) as FlowrAnalyzerPackageVersionsPlugin[]); |
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.
The functions context should not directly rely on the package versions, but maybe be a child of the analyzer dependencies context similar to the loading order
| FlowrAnalyzerPackageVersionsPlugin | ||
| } from '../plugins/package-version-plugins/flowr-analyzer-package-versions-plugin'; | ||
|
|
||
| export enum FunctionTypes { |
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.
it is weird to have a function type symbol, maybe rename this to ExportTypes
| export interface ReadOnlyFlowrAnalyzerFunctionsContext { | ||
| readonly name: string; | ||
|
|
||
| getFunctionInfo(name: string, className?: string): FunctionInfo | undefined; |
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.
the function info must be scoped by the package namespace!
| public addFunctionInfo(functionInfo: FunctionInfo) { | ||
| const fi = this.functionInfo.get(functionInfo.name); | ||
| if(fi) { | ||
| // merge? |
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.
Yes, and at least warn
|
|
||
| public getFunctionInfo(name: string, className?: string): FunctionInfo | undefined { | ||
| if(className) { | ||
| return this.functionInfo.get(`${name}.${className}`); |
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.
not what i expected with className maybe s3TypeDispatch?
| import { FlowrNamespaceFile } from './flowr-namespace-file'; | ||
| import { platformBasename } from '../../../dataflow/internal/process/functions/call/built-in/built-in-source'; | ||
|
|
||
| const NamespaceFilePattern = /^(NAMESPACE(\.txt)?)$/i; |
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 dont think we need the outer parens
| } | ||
|
|
||
| /** | ||
| * |
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.
could you please fill in these?
| process(ctx: FlowrAnalyzerContext): void { | ||
| const nmspcFiles = ctx.files.getFilesByRole(FileRole.Namespace); | ||
| if(nmspcFiles.length !== 1) { | ||
| descriptionFileLog.warn(`Supporting only exactly one NAMESPACE file, found ${nmspcFiles.length}`); |
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.
do we? why? namespace files can be merged, we just have to throw errors if things are incompatible, but we have to that as well if it happens in the same namespace file
| 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.
This is still open!
Uh oh!
There was an error while loading. Please reload this page.