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

feat: omit objects with empty values #24

Merged
merged 3 commits into from Dec 9, 2018

Conversation

Projects
None yet
4 participants
@n1ru4l
Copy link
Contributor

n1ru4l commented Dec 7, 2018

Fixes #23

This is the quickest fix I could find

@janpio

This comment has been minimized.

Copy link
Contributor

janpio commented Dec 7, 2018

Isn't this a bit broad, possibly also removing stuff that should not be removed but fail as the param really should be there or the value really should be set?

We will definitely need some tests around this to be confident enough that this is working as intended I would say :/

@n1ru4l

This comment has been minimized.

Copy link
Contributor

n1ru4l commented Dec 7, 2018

@janpio IMHO it does not make sense to set a value to null/undefined. And if the perpetrator intended to so he could simply set the value to "null" instead of null (or "undefined" instead of undefined.

However I can see your concern about breaking some stuff.

Maybe we could ship this with a option e.g. (dropNullAndUndefined: boolean)?

@n1ru4l

This comment has been minimized.

Copy link
Contributor

n1ru4l commented Dec 7, 2018

@janpio I added some tests and fixed the implementation, also you will now have to pass dropUndefinedOrNull to writeSync, otherwise undefined and null values will be handled how they are atm

@janpio

This comment has been minimized.

Copy link
Contributor

janpio commented Dec 7, 2018

Having tests is great - thanks for making the changes.
I'll let someone with more JS experience review the actual code.

@brodybits

This comment has been minimized.

Copy link
Contributor

brodybits commented Dec 7, 2018

I am not a super expert with Xcode projects, will review the best I can.

I would favor renaming the option to something more meaningful, such as "dropFrameworksWithEmptyPath".

Otherwise it looks good, thanks for contributing the test cases as well.

@n1ru4l

This comment has been minimized.

Copy link
Contributor

n1ru4l commented Dec 7, 2018

@brodybits dropFrameworksWithEmptyPath would be misleading because it changes the writer to drop more than just paths a.k.a every value that is null or undefined.

@brodybits

This comment has been minimized.

Copy link
Contributor

brodybits commented Dec 7, 2018

As I said I am not an Xcode expert so it would help if you could better explain the actual problem and proposed change in behavior.

I find "NullOrUndefined" to be a bit ugly for the exported API. Also "dropUndefinedOrNull" does not give me a clear idea of what it actually does.

@n1ru4l

This comment has been minimized.

Copy link
Contributor

n1ru4l commented Dec 8, 2018

@brodybits Maybe dropUndefinedOrNullValues would be better 😅.

The problem is explained here: #23

My take on the solution is a broader approach, which not only works for the path "property".

In my opinion it makes sense to omit a key from "writing" that has undefined or null as a value instead of setting it to a stringified version of undefined or null.

I do not think that anyone who is currently using this library intends to set any property value to undefined or null. Nevertheless if anyone wants to achieve it he could simply set the value to String(undefined)" or String(null) (which has currently the same effect, like a described in my earlier comment).

However because I like the idea of having no breaking changes in patch/feature versions, I introduced the functionality to omit properties whose value is undefined or null behind an option that has to be enabled first.

Example:

Input Object:

const group = {
  name: "Frameworks",
  path: undefined,
  someOtherKey: null,
  foo: String(undefined),
  bar: String(null)
}

Result without option disabled/current master:

8E63D5C4EFE34B3BAE93815F /* Frameworks */ = {
			name = Frameworks;
			path = undefined;
			someOtherKey = null;
			foo = undefined; 
			bar = null; 
		};

Result with option enabled:

8E63D5C4EFE34B3BAE93815F /* Frameworks */ = {
			name = Frameworks;
			foo = undefined; 
			bar = null;
		};

If you come up with a better name I will rename the option.

Here are some of my suggestions:

  • dropUndefinedOrNull
  • dropUndefinedOrNullValues
  • omitEmptyValues
@brodybits

This comment has been minimized.

Copy link
Contributor

brodybits commented Dec 9, 2018

I would probably favor omitEmptyValues.

@brodybits brodybits referenced this pull request Dec 9, 2018

Open

Major update proposal #20

6 of 11 tasks complete
@n1ru4l

This comment has been minimized.

Copy link
Contributor

n1ru4l commented Dec 9, 2018

@brodybits Done.

@brodybits brodybits changed the title feat: skip undefined or null objects feat: omit objects with empty values Dec 9, 2018

@brodybits brodybits requested a review from dpogue Dec 9, 2018

@dpogue

dpogue approved these changes Dec 9, 2018

Copy link
Member

dpogue left a comment

👍 from me

@brodybits brodybits merged commit 72daa2d into apache:master Dec 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment