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

fix: fix tuistignore when local path are specified #3456

Merged
merged 2 commits into from
Sep 22, 2021
Merged

Conversation

danieleformichelli
Copy link
Collaborator

Short description πŸ“

relative path (as the one we are specifying in Tuist own .tuistignore) were not matched correctly

Checklist βœ…

  • The code architecture and patterns are consistent with the rest of the codebase.
  • The changes have been tested following the documented guidelines.
  • The CHANGELOG.md has been updated to reflect the changes. In case of a breaking change, it's been flagged as such.
  • In case the PR introduces changes that affect users, the documentation has been updated.

Copy link
Collaborator

@adellibovi adellibovi left a comment

Choose a reason for hiding this comment

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

Since this is a more internal feature and we do not have expectations on the syntax of .tuistignore, I would rather change the content of the existing .tuistignore to be projects/tuist/fixtures/** instead of projects/tuist/fixtures rather than adding more logic. What do you think?

@danieleformichelli
Copy link
Collaborator Author

danieleformichelli commented Sep 19, 2021

Not sure, I agree it's internal, but this might change in the future.
I would prefer to keep a syntax similar to the .gitignore to be as less surprising as possible and the additional logic is just a couple of lines.

@adellibovi
Copy link
Collaborator

adellibovi commented Sep 20, 2021

I think if we start adding more behaviour/functionalities we may go into the direction where users would expect this to behave like .gitignore which actually have a lot more of features (like negating logic etc..). Functionality can always be added later, but removing them it is harder.
Said so, I would still personally prefer to adapt our .tuistignore instead of extending the functionality, anyway I will approve the PR πŸ˜ƒ
It would be good to hear other opinions cc @fortmarek @pepibumur

@danieleformichelli
Copy link
Collaborator Author

@adellibovi I agree with your points, but in this case adding a folder would have no effect as the tuistignore works only on files, so it would be weird to have an entry which actually does nothing

@pepicrft
Copy link
Contributor

CHANGELOG entry is missing but I'll update it in main to avoid delaying the merge further.

@pepicrft pepicrft merged commit cf4f4e8 into main Sep 22, 2021
@pepicrft pepicrft deleted the fix/tuistignore branch September 22, 2021 14:45
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.

3 participants