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.

Sign up for GitHub

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

Jump to bottom

Feature/validation with fieldmasks #57

Closed
wants to merge 17 commits into from
Closed

Feature/validation with fieldmasks #57

wants to merge 17 commits into from

Conversation

KrishnaIyer
Copy link

Summary:

Closes TheThingsIndustries#2.

Changes:

  • Remove References to mwitkow
  • Add util functions to extract and evaluate fieldmasks.
  • Modify and clean package
  • Add testing.

Notes for Reviewers:

  • The original tool has a ton of edge cases that are not cleanly handed. I've fitted our functions as much as possible without a big rewrite.

@Helcaraxan
Copy link
Collaborator

This PR slipped through my previous catch-up of outstanding PRs. I am going to reject it on the following basis:

  • There is no clear objective to the changes apart from unidentified general improvements.
  • The change of references of mwitkow to TheThingsIndustries is probably unintentional and not warranted.
  • The PR is too big to be reasonably reviewable.

I am more than happy to take smaller contributions with clear intentions and workable / reviewable scope but in the current state I can't use this unfortunately.

@Helcaraxan Helcaraxan closed this Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Validation with FieldMasks
2 participants
@KrishnaIyer @Helcaraxan