Skip to content

feat: add getContents() for reading external markdown files into table descriptions pr fix#2164

Open
AmmanuelT wants to merge 11 commits into
dataform-co:mainfrom
AmmanuelT:read_description_from_external_file
Open

feat: add getContents() for reading external markdown files into table descriptions pr fix#2164
AmmanuelT wants to merge 11 commits into
dataform-co:mainfrom
AmmanuelT:read_description_from_external_file

Conversation

@AmmanuelT

Copy link
Copy Markdown
Contributor

Closes #1723

Re-Opening Pr #2138, which failed once merged as a node module was being used. It has now been removed.

kolina commented - #2138 (comment)
@AmmanuelT, unfortunately I had to revert your commit in #2155 because it broke compilation in GCP.

I think the reason for breakage is that we use pure V8 for compilation in GCP and we can't use NodeJs modules like path here. Feel free to send a new PR, I think it'll be quite feasible to remove these dependency: you use it for join call here, you can instead use our helper

Changes:

  • core/session.ts : replacing node module usage with Path.join helper function.

@AmmanuelT AmmanuelT requested a review from a team as a code owner May 9, 2026 23:22
@AmmanuelT AmmanuelT requested review from andrzej-grudzien and removed request for a team May 9, 2026 23:22
@kolina

kolina commented May 10, 2026

Copy link
Copy Markdown
Contributor

/gcbrun

Comment thread core/session.ts
Comment thread core/session.ts Outdated
@AmmanuelT

Copy link
Copy Markdown
Contributor Author

Hi @kolina I was busy for a bit so I didn't have time to close this out! I think this is ready for a final review/merge!

@kolina

kolina commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

/gcbrun

Comment thread core/session.ts
const resolvedPath = Path.join(callerDir,filePath);
const absolutePath = Path.separator + Path.normalize(Path.join(this.rootDir, resolvedPath));

if (!absolutePath.startsWith(this.rootDir)){

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous added a separator to this.rootDir if it wasn't present.

Are you sure it's safe to not append it? I'm thinking about such use cases

rootDir = a/b and absolutePath = a/bc.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Read table description from external files

2 participants