Skip to content

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

Open
AmmanuelT wants to merge 10 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 10 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
Copy link
Copy Markdown
Contributor

kolina commented May 10, 2026

/gcbrun

Comment thread core/session.ts
const callerFile = utils.getCallerFile(this.rootDir);
const callerDir = Path.dirName(callerFile);
const resolvedPath = Path.normalize(Path.join(callerDir,filePath));
const absolutePath = Path.separator + Path.normalize(Path.join(this.rootDir, resolvedPath));
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.

Do we need to prepend Path.separator here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The join helper strips the separator from the beginning of the absolutePath, so the start with root wouldn't work because of the missing separator.

Comment thread core/session.ts Outdated
public getContents(filePath: string): string {
const callerFile = utils.getCallerFile(this.rootDir);
const callerDir = Path.dirName(callerFile);
const resolvedPath = Path.normalize(Path.join(callerDir,filePath));
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.

If we call Path.normalize below anyway, here it may not be needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, it is redundant. It has been removed. Thank you for pointing this out!

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