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

[x2cpg] SourceFiles.determine too Coupled with X2CpgConfig #3811

Closed
DavidBakerEffendi opened this issue Nov 9, 2023 · 1 comment
Closed
Labels
bug Something isn't working consistency Concerns abstracting and adding tooling to unify duplicated or inconsistent code help wanted Extra attention is needed

Comments

@DavidBakerEffendi
Copy link
Collaborator

DavidBakerEffendi commented Nov 9, 2023

Original report by @khemrajrathore

Describe the problem

Source Code

We have 4 overloaded variants of the determine function, many of which end up to one or two of the same ones, but the determine function at LNo 57 and LNo 77 doesn’t use the filterFiles function, and it doesn’t seem correct to me.

The function at LNo 57 is used at most places, which is not considering any exclusion rules provided in the field withIgnoredFilesRegex. This leaves the X2CpgConfig silently/unintentionally unutilized.

It is observed that there is a place for determine to be called without X2CpgConfig, but it is mistakenly confused by frontends that ingest source code for processing.

Desired Solution
Rename the function at LNo 57 to something like determineAll and replacing it with the function at LNo 64 where ever it is being called for processing files to be generated by the CPG. As well as inspect usages of LNo77 such that the result of the solution is that all frontends call determine that make use of the config.

Alternatives
Remove the function at LNo 57 and add another overloaded variant where the config is an optional argument so that developers are intentional about using it without X2CpgConfig

Points from @fabsx00:

  • Perhaps the filtering should not exist in determine at all. This adds extra coupling and implicit functionality.
  • Only a subset of the X2CpgConfig properties are used, and by simply passing the whole config, it is unclear what it's being used for.
  • File ingestion and filtering may need a general refactor to be clearer to developers and improve maintainability.
@DavidBakerEffendi DavidBakerEffendi added bug Something isn't working consistency Concerns abstracting and adding tooling to unify duplicated or inconsistent code help wanted Extra attention is needed labels Nov 9, 2023
@DavidBakerEffendi DavidBakerEffendi changed the title [x2cpg] withIgnoredFilesRegex Unused Due to Many Overloaded SourceFiles.determines [x2cpg] SourceFiles.determine too Coupled with X2CpgConfig Nov 9, 2023
@DavidBakerEffendi
Copy link
Collaborator Author

Resolved by #3813 and #3822

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working consistency Concerns abstracting and adding tooling to unify duplicated or inconsistent code help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant