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

Utility function added to check permissions on directory #95

Merged

Conversation

eliasyishak
Copy link
Contributor

@eliasyishak eliasyishak commented May 19, 2023

Addresses issue:

Issue filed in the flutter/flutter repo:

This makes use of the Directory's statSync() method along with the modeString() method on FileStat to return the permissions set on a given directory and checks for write permissions. If there are no permissions to write to a user's home directory, the factory constructor will return a NoOp implementation


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

Many Dart repos have a weekly cadence for reviewing PRs - please allow for a week or two of latency for initial review feedback.


@eliasyishak eliasyishak marked this pull request as ready for review May 30, 2023 15:17
@@ -4,6 +4,7 @@
- Remove the `pddFlag` now that the revisions to the PDD have been finalized to persist data in the log file and session json file
- Opting out will now delete the contents of the CLIENT ID, session json, and log files; opting back in will regenerate them as events send
- `enableAsserts` parameter added to constructors for `Analytics` to check body of POST request for Google Analytics 4 limitations
- Now checking if write permissions are enabled for user's home directory, if not allowed, `NoOpAnalytics` returned by `Analytics` factory constructor
Copy link
Contributor

Choose a reason for hiding this comment

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

was 1.1.1 already published?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be once this PR is merged into master, I made the mistake of not adding the suffix. In the future, I will use separate feature branches or have the suffix at HEAD for master until it is ready to get published

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, sounds good. I would recommend using # NEXT in the changelog before you're actually ready to publish, unless you know there's a breaking change that would necessitate a major version bump.

Speaking of which, should the next version be 2.0? b55f0d4#diff-e07cea1b47833ed9526208c8d5cf51ea10672e065ca4aa9213a4cd19d5b8d7bf looks breaking.

Copy link
Contributor Author

@eliasyishak eliasyishak May 31, 2023

Choose a reason for hiding this comment

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

Yeah I suppose that is true, the reason we had those overrides at first in the default constructor was so that I could override them in the test/pdd_approved_test.dart. I'm also certain that only the analysis server and flutter cli are the only users of this package and I know they didn't provide overrides.

But I will go ahead and make the version 2.0.0 since it definitely would be a breaking change if those overrides were provided for some reason

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants