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

directory modelling suggestion #130

Merged
merged 1 commit into from
Nov 3, 2022

Conversation

entunidotdeb
Copy link
Contributor

@entunidotdeb entunidotdeb commented Aug 13, 2022

proposing something like this (or better & complete) to have all properties of directory like includeHiddenFiles, maxDepth etc. ik this is quite a big change and would not be backward compatible but i guess worth to include something similar.
your thoughts on this? @AvdLee

@wetransferplatform
Copy link
Collaborator

wetransferplatform commented Aug 14, 2022

Messages
📖

View more details on Bitrise

📖 DiagnosticsTests: Executed 33 tests (0 failed, 0 retried, 0 skipped) in 0.482 seconds

SwiftLint found issues

Severity File Reason
Warning DirectoryTreeReporter.swift:18 Lines should not have trailing whitespace. (trailing_whitespace)
Warning DirectoryTreeReporter.swift:78 Lines should not have trailing whitespace. (trailing_whitespace)

Code Coverage Report

Name Coverage
Diagnostics 70.42% ⚠️

Generated by 🚫 Danger Swift against 105b104

@BasThomas
Copy link

Thanks for this, @entunidotdeb! Can you provide a bit more rationale for this change?

to have all properties of directory like includeHiddenFiles, maxDepth etc

What makes this an improvement over the current API, for example?

Also given that this would break the API, it'd technically require a major version bump, so that's at least something to keep in mind.

@entunidotdeb
Copy link
Contributor Author

entunidotdeb commented Aug 15, 2022

@BasThomas so currently for a client to have multiple doc reports of different maxDepth, includeHiddenFiles and other properties, client would have to create multiple instances of DirectoryTreesReporter. Even then client would have no option to specifically set maxDepth for a group out of the nested Group/Directory Tree. How about client has option to customize the Directory specific props instead of the DirectoryTreesReporter props ?

multiple DirectoryTreesReporter would lead to multiple DiagnosticsChapter

@BasThomas
Copy link

Gotcha, thanks!

Copy link
Contributor

@AvdLee AvdLee left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution! I like the change. I do believe we can improve this implementation by adding a deprecated initializer for the current implementation, so we allow our users to gracefully migrate.

@entunidotdeb
Copy link
Contributor Author

@AvdLee @BasThomas
I have updated this PR with new init for DirectoryTreeReporter and deprecating the earlier init.
you may review and suggest changes if needed.

Copy link
Contributor

@AvdLee AvdLee left a comment

Choose a reason for hiding this comment

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

Thanks, we're getting closer! Just a few points left 💪

@kairadiagne kairadiagne removed their request for review September 2, 2022 13:12
@AvdLee
Copy link
Contributor

AvdLee commented Sep 15, 2022

@entunidotdeb the code looks great, but it seems that tests are failing. Did you manage to run tests successfully locally?

@entunidotdeb
Copy link
Contributor Author

@entunidotdeb the code looks great, but it seems that tests are failing. Did you manage to run tests successfully locally?

@AvdLee yes sir

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity. Remove the Stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Oct 20, 2022
AvdLee
AvdLee previously approved these changes Oct 24, 2022
@AvdLee
Copy link
Contributor

AvdLee commented Oct 24, 2022

@entunidotdeb our repository requires signed commits. Would it be possible for you to enable signed commits and recommit using the following command:

 git rebase --exec 'git commit --amend --no-edit -n -S' -i develop
 git push --force

You can read more about setting up signed commits here.

@github-actions github-actions bot removed the Stale label Oct 25, 2022
@entunidotdeb entunidotdeb force-pushed the feature/DirectoryModeled branch 2 times, most recently from 5cf5795 to 69fefa1 Compare October 26, 2022 12:29
AvdLee
AvdLee previously approved these changes Oct 31, 2022
@AvdLee
Copy link
Contributor

AvdLee commented Oct 31, 2022

@entunidotdeb we're almost there! Your commits are signed but unverified:

CleanShot 2022-10-31 at 09 50 58@2x

You can learn more about that here: https://docs.github.com/en/authentication/managing-commit-signature-verification/displaying-verification-statuses-for-all-of-your-commits

@entunidotdeb
Copy link
Contributor Author

@AvdLee Done

Copy link
Contributor

@raphkoebraam raphkoebraam left a comment

Choose a reason for hiding this comment

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

Thanks for going over the comments @entunidotdeb! 🚀

@entunidotdeb
Copy link
Contributor Author

Thanks for going over the comments @entunidotdeb! 🚀

@entunidotdeb entunidotdeb reopened this Nov 1, 2022
@AvdLee
Copy link
Contributor

AvdLee commented Nov 3, 2022

@entunidotdeb congrats with your successful contribution! Thanks again for setting up signed commits 🙏

@AvdLee AvdLee merged commit 8861bd6 into WeTransfer:master Nov 3, 2022
@entunidotdeb
Copy link
Contributor Author

Thank you @AvdLee for suggestions on PR, approval and merge.
Thanks @peagasilva for approval.

@wetransferplatform
Copy link
Collaborator

Congratulations! 🎉 This was released as part of Release 4.3.1 🚀

Generated by GitBuddy

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.

5 participants