feat: consistent function naming with underscores

[norbitrial]

This PR is a solution for #793 issue.

Function calls are tested after renaming and working exact the same way as before. There are extra warnings on the console for old function calls for the following cases:

• groupby() function call,
• fromlast() and tofirst() calls.

Let me know if you have any suggestions. Thank you! 🙇 👍

[DmitrySharabin]

That's awesome! Thank you. Actually, you already did a great job; there is only one question left. Please see the comments.

[DmitrySharabin]

That's awesome! Thank you. Actually, you already did a great job; there is only one question left. Please see the comments.

[DmitrySharabin]

By the way, it would be nice if you could also make the corresponding changes to our testsuite so that it reflects the changes to the API. To be precise, to this file. But that's not necessary. I can do it too. 😀

[LeaVerou]

By the way, it would be nice if you could also make the corresponding changes to our testsuite so that it reflects the changes to the API. To be precise, to this file. But that's not necessary. I can do it too. 😀

Actually, it's even more important to make the changes in our docs. (which should also include both versions, with e.g. groupBy just saying something like "Name of group_by() before v0.3.0", with a note about which Mavo version supports what)

[DmitrySharabin]

@LeaVerou, it seems to me this PR is also worth merging before v0.3.0 is released. It will allow us to be consistent with the new functions (e.g., readable_datetime()). @norbitrial did a great job. There is a couple of minor issues left I can fix myself.

[netlify]

✅ Deploy Preview for getmavo ready!

Name	Link
🔨 Latest commit	2e6e578
🔍 Latest deploy log	https://app.netlify.com/sites/getmavo/deploys/6536bdfeee73e700072a864d
😎 Deploy Preview	https://deploy-preview-821--getmavo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[LeaVerou]

LGTM, my only suggestion would be to abstract the deprecation code away into a function instead of repeating it.

Something like:

function deprecatedFunction(name, oldName, fn = _[name]) {
	return function (...args) {
		Mavo.warn(`${oldName}() is deprecated and will be removed in the next version of Mavo. Please use ${name}() instead.");
		return fn(...args);
	}
}

...and to make the docs and tests edits!

In fact, in https://mavo.io/docs/functions/ I think we should have two entries for each of these, with the old one saying "Deprecated alias of xxx()"

[DmitrySharabin]

Done.

[LeaVerou]

Can we not remove extra commas please?

a) trailing commas are there for a reason, it means that adding things after is less likely to result in a syntax error
b) even if we wanted to remove trailing commas, this PR is not the place and it increases the size of the diff making it harder to review

Also, this abstracted away the deprecation code, but is not using it in all three places where it's needed.
We should expose it somewhere, so that other modules can use it as well (or even plugins).

[DmitrySharabin]

Can we not remove extra commas please?

Sorry about that. I thought they were some leftovers from a code formatter (e.g., Prettier). Fixed it.

Also, this abstracted away the deprecation code, but is not using it in all three places where it's needed. We should expose it somewhere, so that other modules can use it as well (or even plugins).

I hesitated to put it into utils.js and made the wrong choice. I should have asked. From my point of view, utils.js might be a good place for the deprecatedFunction() function. If not, I'm open to making other changes if needed.

[LeaVerou]

Can we not remove extra commas please?

Sorry about that. I thought they were some leftovers from a code formatter (e.g., Prettier). Fixed it.

Also, this abstracted away the deprecation code, but is not using it in all three places where it's needed. We should expose it somewhere, so that other modules can use it as well (or even plugins).

I hesitated to put it into utils.js and made the wrong choice. I should have asked. From my point of view, utils.js might be a good place for the deprecatedFunction() function. If not, I'm open to making other changes if needed.

Yeah, I can't think of a better place either, so utils it is. Or Mavo.Script. Up to you!

[DmitrySharabin]

Yeah, I can't think of a better place either, so utils it is. Or Mavo.Script. Up to you!

Then, I believe it's ready for your review. Again. 😅

[LeaVerou]

Actually I know where it should live! On Mavo.Functions.util! Apologies, I forgot about that entirely.

[DmitrySharabin]

Actually I know where it should live! On Mavo.Functions.util! Apologies, I forgot about that entirely.

Honestly, I was also trying to “house” deprecatedFunction() there (it was one of my first attempts). However, I faced an issue I didn't (and still don't) know how to resolve elegantly.

Suppose we have the following code (where $u is an alias for _.util):

fromlast: $u.deprecatedFunction("from_last", "fromlast")

And since util is defined below and we are trying to use something that is not yet defined, we get an error TypeError: Cannot read properties of undefined (reading 'util').

I suspect that we can do something like this (and it works)

fromlast: (...args) => $u.deprecatedFunction("from_last", "fromlast").call(this, ...args)

But I'm unsure whether it's the correct way to do this. What am I missing?

[LeaVerou]

Actually I know where it should live! On Mavo.Functions.util! Apologies, I forgot about that entirely.

Honestly, I was also trying to “house” deprecatedFunction() there (it was one of my first attempts). However, I faced an issue I didn't (and still don't) know how to resolve elegantly.

Suppose we have the following code (where $u is an alias for _.util):

fromlast: $u.deprecatedFunction("from_last", "fromlast")

And since util is defined below and we are trying to use something that is not yet defined, we get an error TypeError: Cannot read properties of undefined (reading 'util').

I suspect that we can do something like this (and it works)

fromlast: (...args) => $u.deprecatedFunction("from_last", "fromlast").call(this, ...args)

But I'm unsure whether it's the correct way to do this. What am I missing?

Just move it up?

E.g.

let $u = {
	/* utils here */
};

Mavo.Functions = {
	/* ... */
	util: $u
}

[DmitrySharabin]

Just move it up?

It worked! 🥳 Thank you.

[DmitrySharabin]

@norbitrial, thank you so much for your work!