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

Remove trailing slashes from source paths (#6102) #6179

Merged
merged 4 commits into from
Feb 21, 2023
Merged

Remove trailing slashes from source paths (#6102) #6179

merged 4 commits into from
Feb 21, 2023

Conversation

jmg-duarte
Copy link
Contributor

@jmg-duarte jmg-duarte commented Oct 30, 2022

resolves #6102

Description

Paths like "models" and "models/" are the same but were resolved as different. Leading to issues when seemingly equivalent (but different) paths were used in (for example) seeds and models.

The following example would detect the same files twice.

model-paths:
  - models
seed-paths:
  - models/

Checklist

@jmg-duarte jmg-duarte requested review from a team as code owners October 30, 2022 10:21
@cla-bot cla-bot bot added the cla:yes label Oct 30, 2022
@lostmygithubaccount lostmygithubaccount added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Oct 30, 2022
@jmg-duarte jmg-duarte closed this by deleting the head repository Dec 31, 2022
@jmg-duarte jmg-duarte reopened this Jan 16, 2023
@peterallenwebb
Copy link
Contributor

@jmg-duarte, thanks for this contribution.

It appears that to make mypy happy, which is necessary before merge, you will need to avoid assigning the paths variable twice, but with objects of different types. Overall the fix makes sense to me, and I'll keep an eye out for your update so that we can quickly merge.

@jmg-duarte jmg-duarte closed this Jan 31, 2023
@jmg-duarte jmg-duarte reopened this Jan 31, 2023
@jmg-duarte
Copy link
Contributor Author

Hey @peterallenwebb, I'm having some trouble updating the branch because I deleted the forked repo a while ago by mistake and haven't been able to push changes to the branch. I've followed guides but to no avail. I've also contacted GitHub support to see if I can avoid creating a new PR but if you feel this is important enough, I can go one to close this one and create a new branch.

@peterallenwebb
Copy link
Contributor

@jmg-duarte That would be great! If don't feel like going through all the trouble, I can also get this change in on your behalf with no further effort on your part, just let me know.

@cla-bot
Copy link

cla-bot bot commented Feb 1, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: José Duarte.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot cla-bot bot added cla:yes and removed cla:yes labels Feb 1, 2023
@jmg-duarte
Copy link
Contributor Author

@peterallenwebb GitHub support was super helpful and I got it working. I've applied the changes, let me know if there is something I should update further!

Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

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

A few formatting issues and you'll need to resolve the conflict. Then this should be good to merge in!

core/dbt/config/project.py Outdated Show resolved Hide resolved
core/dbt/config/project.py Outdated Show resolved Hide resolved
core/dbt/config/project.py Outdated Show resolved Hide resolved
@jmg-duarte
Copy link
Contributor Author

A few formatting issues and you'll need to resolve the conflict. Then this should be good to merge in!

Rebasing over main was enough, thank you! Resolved and ready for review!

Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

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

Almost there! Just need to fix mypy.

Comment on lines 142 to 143
paths = map(lambda s: s.rstrip("/"), paths)
return list(set(paths))
Copy link
Member

Choose a reason for hiding this comment

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

my doesn't like that you change the type of paths here

Suggested change
paths = map(lambda s: s.rstrip("/"), paths)
return list(set(paths))
paths_stripped = map(lambda s: s.rstrip("/"), paths)
return list(set(paths_stripped))

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 did this before, with the constant updates to the underlying code I must've messed up the rebase. Let me fix that

Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

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

Clicked the wrong button, meant to request a change!

@cla-bot
Copy link

cla-bot bot commented Feb 20, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: José Duarte.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot cla-bot bot removed the cla:yes label Feb 20, 2023
@cla-bot cla-bot bot added the cla:yes label Feb 20, 2023
@jmg-duarte jmg-duarte removed the request for review from Fleid February 20, 2023 11:59
Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

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

@jmg-duarte almost there! black now fails on a formatting issue. If you run pre-commit (pre-commit run after installing it as described in the contributing doc), it will make the fixes for you. Alternatively, you can check out the failing tests and manually fix the formatting issue.

@cla-bot
Copy link

cla-bot bot commented Feb 21, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: José Duarte.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot cla-bot bot removed the cla:yes label Feb 21, 2023
@jmg-duarte
Copy link
Contributor Author

Should be working now. My VSCode applied isort which in turn f'd up the black formatting. Thank you for the patience

@cla-bot cla-bot bot added the cla:yes label Feb 21, 2023
Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @jmg-duarte for the contribution. I'll get it merged in!

@emmyoop emmyoop merged commit 7667784 into dbt-labs:main Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Duplicated source when seed and model paths overlap
5 participants