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

Sweep: Hovering on a macro should show more details like description and arguments #1179

Merged
merged 3 commits into from
May 30, 2024

Conversation

sweep-ai[bot]
Copy link
Contributor

@sweep-ai sweep-ai bot commented May 30, 2024

Description

This pull request introduces enhancements to the hover functionality in the extension, specifically targeting macros. It adds the ability to display detailed information such as the macro's description and its arguments when a user hovers over a macro reference in the code. This popup will show information like referenced by and depends on with right nodes. On clicking the node, user will be taken to the right file in vscode editor

Summary

  • Added a new MacroHoverProvider class to handle hover events for macros, displaying a hover card with the macro's description and arguments.
  • Integrated the MacroHoverProvider into the HoverProviders class to register it alongside existing hover providers.
  • Enhanced the MacroParser to extract and store additional metadata about macros, including their descriptions and arguments, during the parsing process.
  • Created a utility function generateMacroHoverMarkdown (not shown in the diffs but referenced in the code) to format the hover content as Markdown.
  • This update enriches the information available to developers directly in their editor, facilitating a more efficient development workflow when working with macros in DBT projects.

Fixes #1178.

Demo: https://www.loom.com/share/c5d5554f615d48229444f29afdf14357?sid=9e14d668-cefd-47e2-9fcc-ea9c2138e457

💡 To get Sweep to edit this pull request, you can:

  • Comment below, and Sweep can edit the entire PR
  • Comment on a file, Sweep will only modify the commented file
  • Edit the original issue to get Sweep to recreate the PR from scratch

This is an automated message generated by Sweep AI.

Copy link
Contributor Author

sweep-ai bot commented May 30, 2024

Rollback Files For Sweep

  • Rollback changes to src/manifest/parsers/macroParser.ts
  • Rollback changes to src/hover_provider/macroHoverProvider.ts
  • Rollback changes to src/hover_provider/index.ts

This is an automated message generated by Sweep AI.

Comment on lines 64 to 81
let description = "";
let args = [];

// Parse macro description
for (let i = index - 1; i >= 0; i--) {
const line = macroFileLines[i].trim();
if (line.startsWith("/*") || line.startsWith('"""')) {
description = line.slice(2, -2).trim();
break;
}
}

// Parse macro arguments
const argsMatch = currentLine.match(/\((.*)\)/);
if (argsMatch) {
args = argsMatch[1].split(",").map(arg => arg.trim());
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of this change, you could get description and arguments from macro in line no 37

@saravmajestic saravmajestic requested review from mdesmet, AdiGajbhiye, anandgupta42 and saravmajestic and removed request for saravmajestic May 30, 2024 12:59
Copy link
Contributor Author

sweep-ai bot commented May 30, 2024

Sweep: PR Review

Authors of pull request: @saravmajestic, @sweep-ai[bot]

This pull request added hover functionality for macros in the DBT SQL editor.

The MacroMetaData interface in src/domain.ts was updated to include new properties such as description, arguments, and name, which provide additional metadata for macros. This interface was also exported to be used in other modules.

A new MacroHoverProvider class was created in src/hover_provider/macroHoverProvider.ts. This class implements the HoverProvider interface and provides hover information for macros by querying metadata and generating markdown content.

The HoverProviders class in src/hover_provider/index.ts was updated to include the MacroHoverProvider as a dependency and register it as a hover provider for the DBT SQL language selector. This ensures that hover information for macros is available in the editor.

A new function generateMacroHoverMarkdown was added to src/hover_provider/utils.ts. This function generates the markdown content for the macro hover tooltips, including the macro's name, description, and arguments.

Finally, the MacroParser class in src/manifest/parsers/macroParser.ts was updated to include the new properties (description, arguments, and name) in the macro metadata map, ensuring that the hover provider has access to the necessary information.


Sweep Found These Issues

src/domain.ts
  • The new name property in the MacroMetaData interface is mandatory, which could break existing code that does not provide this property.
  • export interface MacroMetaData {
    path: string | undefined; // in dbt cloud, packages are not downloaded locally
    line: number;
    character: number;
    uniqueId: string;
    description?: string;
    arguments?: { name: string; type: string; description: string }[];
    name: string;
    depends_on: DependsOn;

    View Diff

src/hover_provider/index.ts
  • The macroHoverProvider is registered twice as a hover provider for the DBTPowerUserExtension.DBT_SQL_SELECTOR, which could lead to duplicate hover information being provided.
  • this.disposables.push(
    languages.registerHoverProvider(
    DBTPowerUserExtension.DBT_SQL_SELECTOR,
    this.macroHoverProvider,
    ),
    );

    View Diff

src/hover_provider/macroHoverProvider.ts
  • The provideHover method does not handle potential exceptions from this.queryManifestService.getEventByDocument(document.uri), which could cause the hover functionality to fail silently.
  • token: CancellationToken,
    ): ProviderResult<Hover> {
    const hoverText = document.getText(

    View Diff

  • The provideHover method does not handle the case where document.getWordRangeAtPosition(position) returns undefined, which could lead to a runtime error.

  • View Diff

  • Sweep has identified a redundant function: The new function is redundant because its functionality is already implemented in the dispose methods of the DefinitionProviders, WebviewViewProviders, AutocompletionProviders, and TreeviewProviders classes.
  • constructor(
    private telemetry: TelemetryService,
    private dbtTerminal: DBTTerminal,
    private queryManifestService: QueryManifestService,
    ) {}
    dispose() {

    View Diff

  • Sweep has identified a redundant function: The new function is redundant because its functionality is already fully implemented in the provideHover method of the MacroHoverProvider class in macroHoverProvider.ts.
  • const x = this.disposables.pop();
    if (x) {
    x.dispose();
    }
    }
    }
    provideHover(
    document: TextDocument,
    position: Position,
    token: CancellationToken,
    ): ProviderResult<Hover> {
    const hoverText = document.getText(
    document.getWordRangeAtPosition(position),
    );
    this.dbtTerminal.debug("MacroHoverProvider", `checking: ${hoverText}`);
    const eventResult = this.queryManifestService.getEventByDocument(
    document.uri,
    );
    if (!eventResult) {
    return;
    }
    const { macroMetaMap, nodeMetaMap } = eventResult;
    const macroMeta = macroMetaMap.get(hoverText);
    if (!macroMeta) {

    View Diff

src/hover_provider/utils.ts
  • Sweep has identified a redundant function: The new function generateMacroHoverMarkdown is redundant as its functionality is already covered by generateHoverMarkdownString with minor modifications.
  • }
    export const generateMacroHoverMarkdown = (
    node: MacroMetaData,
    referencedBy: (MacroMetaData | NodeMetaData)[],
    event: ManifestCacheProjectAddedEvent,
    ) => {
    const content = new MarkdownString();
    content.supportHtml = true;
    content.isTrusted = true;
    content.appendMarkdown(
    `<span style="color:#347890;">(Macro)&nbsp;</span><span><strong>${node.name}</strong></span>`,
    );
    if (node.description !== "") {
    content.appendMarkdown(`</br><span>${node.description}</span>`);
    }
    addSeparator(content);
    node.arguments?.forEach((macroArg) => {
    content.appendMarkdown(
    `<span style="color:#347890;">(argument)&nbsp;</span><span>${macroArg.name} &nbsp;</span>`,
    );
    if (macroArg.type !== null) {
    content.appendMarkdown(
    `<span>-&nbsp;${macroArg.type.toLowerCase()}</span>`,
    );
    }
    if (macroArg.description !== "") {
    content.appendMarkdown(
    `<br/><span><em>${macroArg.description}</em></span>`,
    );
    }
    content.appendMarkdown("</br>");
    });

    View Diff

src/manifest/parsers/macroParser.ts

@anandgupta42 anandgupta42 merged commit c1dac48 into main May 30, 2024
1 check passed
anandgupta42 added a commit that referenced this pull request May 30, 2024
…and arguments (#1179) (#1180)

* feat: Updated 3 files

* fix: macro hover provider

* fix: popup ui

---------

Co-authored-by: sweep-ai[bot] <128439645+sweep-ai[bot]@users.noreply.github.com>
Co-authored-by: saravmajestic <sarav.majestic@gmail.com>
mdesmet pushed a commit that referenced this pull request May 31, 2024
#1182)

* Sweep: Hovering on a macro should show more details like description and arguments (#1179)

* feat: Updated 3 files

* fix: macro hover provider

* fix: popup ui

---------

Co-authored-by: sweep-ai[bot] <128439645+sweep-ai[bot]@users.noreply.github.com>
Co-authored-by: saravmajestic <sarav.majestic@gmail.com>

* Remove unused 'type' param from model lookup

* fix: right path for compiled/run files

---------

Co-authored-by: sweep-ai[bot] <128439645+sweep-ai[bot]@users.noreply.github.com>
Co-authored-by: saravmajestic <sarav.majestic@gmail.com>
Co-authored-by: anandgupta42 <93243293+anandgupta42@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants