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

Vale: test with several files #5934

Closed
wants to merge 44 commits into from
Closed

Vale: test with several files #5934

wants to merge 44 commits into from

Conversation

nghi-ly
Copy link
Contributor

@nghi-ly nghi-ly commented Aug 15, 2024

What are you changing in this pull request and why?

Testing linter with several files

@nghi-ly nghi-ly requested review from dataders and a team as code owners August 15, 2024 20:38
Copy link

vercel bot commented Aug 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs-getdbt-com ✅ Ready (Inspect) Visit Preview Sep 27, 2024 11:20am

@github-actions github-actions bot added content Improvements or additions to content developer blog This content fits on the developer blog. guides Knowledge best suited for Guides size: large This change will more than a week to address and might require more than one person Docs team Authored by the Docs team @dbt Labs labels Aug 15, 2024
Copy link
Contributor Author

@nghi-ly nghi-ly left a comment

Choose a reason for hiding this comment

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

this test with many edited files is a bit interesting, @mirnawong1 .

  • Vale didn't flag anything on 2 (out of 3) files. i expected for some lines to be flagged. i wonder if this has something to do with "reviewdog" erroring out.
  • Vale flagged several lines in the 3rd file but there were some false positives.

Copy link
Contributor

❗️Oh no, some Vale linting found issues! Please check the Files change tab for detailed results and make the necessary updates.

➡️ Link to detailed report: Files changed

Copy link
Contributor Author

@nghi-ly nghi-ly left a comment

Choose a reason for hiding this comment

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

@mirnawong1 : fantastic work on integrating Vale! left some thoughts/questions for ya

happy to further discuss any of my comments or any questions you may have


<!-- truncate -->
Since the early days of dbt, folks have been interested having MSFT data platforms. Huge shoutout to [Mikael Ene](https://github.com/mikaelene) and [Jacob Mastel](https://github.com/jacobm001) for their efforts back in 2019 on the original SQL Server adapters ([dbt-sqlserver](https://github.com/dbt-msft/dbt-sqlserver) and [dbt-mssql](https://github.com/jacobm001/dbt-mssql), respectively)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

looking at the typo warnings below. curious if there's an easy way to make this list shorter. totally non-blocking.

is it easy for any tech writer to add to our custom dictionary? just wondering whether it's a good idea for each person to be responsible for it 🤔


Many data teams currently use Azure Synapse dedicated pools. However, Fabric Synapse Data Warehouse is the future of data warehousing in the Microsoft Ecosystem. Azure Synapse Analytics will remain available for a few more years, but Microsoft’s main focus is on Fabric as we can see in their roadmap and launches.

Because data platform migrations are complex and time-consuming, it’s perfectly reasonable to still be using dbt with Azure Synapse for the next two years while the migration is under way. Thankfully, if your team already is using ASADSP, transitioning to the new Cloud offering will be much more straightforward than the migration from on-premise databases to the Cloud.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

curious, i wonder if there are more false positives when linting words that are acronyms.

wonder if it would be better -- and if it's easy to do -- to just ignore acronyms.


:::tip
💁🏻‍♀️ If your BI tool allows it, make sure to do the BI-related steps above **in a development environment**. If it doesn’t have these capabilities, stick with duplicating the data product you’re re-building and perform this there so you can swap it later after you’ve tested it thoroughly.
However, if you must use ADF as the deployment pipeline, it is possible to use dbt Cloud APIs. Running dbt Core within Azure Data Factory can be challenging as there’s no easy way to install and invoke dbt Core, because there’s no easy way to install and run Python. The workarounds aren’t great, for example: Setting up dbt calls via Azure Serverless Functions and invoking them from ADF.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we can add "APIs" in our custom dictionary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i noticed that "... invoking them from ADF" has an extra space btn them and from.

is this something Vale can check and flag? hmm, though it may pick up more false positives 🤔

<Lightbox src="/img/snowflake_tutorial/dbt_cloud_setup_snowflake_connection_start.png" title="dbt Cloud - Choose Snowflake Connection" />

4. Enter your **Settings** for Snowflake with:
* **Account** &mdash; Find your account by using the Snowflake trial account URL and removing `snowflakecomputing.com`. The order of your account information will vary by Snowflake version. For example, Snowflake's Classic console URL might look like: `oq65696.west-us-2.azure.snowflakecomputing.com`. The AppUI or Snowsight URL might look more like: `snowflakecomputing.com/west-us-2.azure/oq65696`. In both examples, your account will be: `oq65696.west-us-2.azure`. For more information, see [Account Identifiers](https://docs.snowflake.com/en/user-guide/admin-account-identifier.html) in the Snowflake docs.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Snowflake should probably be in our custom dictionary, i think

website/docs/guides/bigquery-qs.md Show resolved Hide resolved
website/docs/guides/bigquery-qs.md Show resolved Hide resolved

## Controlling table expiration

By default, dbt-created tables never expire. You can configure certain model(s)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, not sure why Vale is flagging this as a typo

Copy link
Contributor

❗️Oh no, some Vale linting found issues! Please check the Files change tab for detailed results and make the necessary updates.

➡️ Link to detailed report: Files changed

@dbt-labs dbt-labs deleted a comment from github-actions bot Sep 27, 2024
Copy link
Contributor

❗️Oh no, some Vale linting found issues! Please check the Files change tab for detailed results and make the necessary updates.

➡️ Link to detailed report: Files changed

Copy link
Contributor

❗️Oh no, some Vale linting found issues! Please check the Files change tab for detailed results and make the necessary updates.

➡️ Link to detailed report: Files changed

Copy link
Contributor

❗️Oh no, some Vale linting found issues! Please check the Files change tab for detailed results and make the necessary updates.

➡️ Link to detailed report: Files changed

Copy link
Contributor

❗️Oh no, some Vale linting found issues! Please check the Files change tab for detailed results and make the necessary updates.

➡️ Link to detailed report: Files changed

Copy link
Contributor

❗️Oh no, some Vale linting found issues! Please check the Files change tab for detailed results and make the necessary updates.

➡️ Link to detailed report: Files changed


Indexes are not supported by Microsoft Fabric Synapse Data Warehouse. Any Indexes provided as a configuration is ignored by the adapter.
Learn more about these parameters in BigQuery's docs:
- [materialized_view_option_list](https://cloud.google.com/bigquery/docs/reference/standard-sql/data-definition-language#materialized_view_option_list)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this shouldn't be flagged as a typo.

weird, the parameter names in the table above aren't getting flagged. oh i see the difference! the square brackets

@mirnawong1 mirnawong1 closed this Oct 4, 2024
@mirnawong1
Copy link
Contributor

closing as testing done and approved by Ly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Improvements or additions to content developer blog This content fits on the developer blog. Docs team Authored by the Docs team @dbt Labs guides Knowledge best suited for Guides size: large This change will more than a week to address and might require more than one person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants