WeTransfer / Diagnostics

Allow users to easily share Diagnostics with your support team to improve the flow of fixing bugs.
MIT License
933 stars 53 forks source link

directory modelling suggestion #130

Closed entunidotdeb closed 1 year ago

entunidotdeb commented 1 year ago

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 commented 1 year ago
Messages
:book: View more details on Bitrise
:book: 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 :no_entry_sign: Danger Swift against 105b10469e4d727460c5df9b9588ea3b09ace26b

BasThomas commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

Gotcha, thanks!

entunidotdeb commented 1 year ago

@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.

AvdLee commented 1 year ago

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

entunidotdeb commented 1 year ago

@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[bot] commented 1 year ago

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.

AvdLee commented 1 year ago

@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.

AvdLee commented 1 year ago

@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 commented 1 year ago

@AvdLee Done

entunidotdeb commented 1 year ago

Thanks for going over the comments @entunidotdeb! 🚀

AvdLee commented 1 year ago

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

entunidotdeb commented 1 year ago

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

wetransferplatform commented 1 year ago

Congratulations! :tada: This was released as part of Release 4.3.1 :rocket:

Generated by GitBuddy