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

Do some variable name cleanup in lift-json. #1519

Merged
merged 3 commits into from Mar 30, 2014

Conversation

Projects
None yet
2 participants
@farmdawgnation
Copy link
Member

farmdawgnation commented Feb 15, 2014

We have a memo named declaredFields and another named declaredFieldsMap, that was added recently to prevent us from having to reflect on each retrieval of the declared fields. These names are a bit confusing, so let's change them up.

I'm thinking hasDeclaredFieldsMemo and declaredFieldsMemo.

@farmdawgnation farmdawgnation added this to the 2.6-M3 milestone Feb 15, 2014

@farmdawgnation farmdawgnation added the JSON label Feb 15, 2014

@farmdawgnation farmdawgnation self-assigned this Feb 15, 2014

Clarify some of the Memo names used in Meta.Reflection.
I noticed when we merged a PR that the names of the various Memos in
Meta.Reflection were getting a bit confusing, so I've cleaned them up a
bit to 1) reduce confusion between what are now called
hasDeclaredFieldsMemo and declaredFieldsMemo and 2) to clarify what the
primaryConstructorArgumentsMemo actually contained.
@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Mar 1, 2014

Since Memo is an internal thing here, thoughts on making Memo take a function that actually populates the memo for a given entry, and have the things that call e.g. hasDeclaredFields call the memo directly instead? I'm thinking a pattern kind of like RequestVar, something like:

  private val classHasDeclaredField = new Memo[(Class[_], String), Boolean]({
    case (clazz, name) =>
      try {
        clazz.getDeclaredField(name)
        true
      } catch {
        case e: NoSuchFieldException => false
      }
  })

Used as:

  def hasDeclaredField(clazz: Class[_], name: String): Boolean = classHasDeclaredField((clazz, name))

Still not 100% clean, but I think it's a little cleaner, and puts the thing that the memo is memoizing in the same place as the memo itself.

Thoughts?

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Mar 1, 2014

Oh and also… hasDeclaredField is no longer being used. Looks like #1517 also removed the call to it, so it's dead code now heh.

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Mar 16, 2014

@farmdawgnation any updates on this?

@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented Mar 16, 2014

Lol. Yeah, completely forgot this existed. Adding a reminder to myself to update it so we can get it merged.

@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented Mar 16, 2014

Will (hopefully) get to it tomorrow or Tuesday.

@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented Mar 17, 2014

I looked at the change you suggested. I like it. I started doing it but it's more than a simple cut-and-paste. Some of the code that is passed to populate the memo uses variables generated inside the functions where the memo is called, meaning I'd have to take a deeper look at how the code in there is architected to make it happen. Deeper than I want to do in this particular PR this evening.

What do you say we merge this and create a ticket for looking at implementing those changes?

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Mar 17, 2014

I'm down.

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Mar 17, 2014

I'll try and have a look/merge later tonight.

@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented Mar 21, 2014

Ping. ;)

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Mar 30, 2014

Tests passing, code looks good, this guy looks good to go. 👍

Shadowfiend added a commit that referenced this pull request Mar 30, 2014

Merge pull request #1519 from lift/msf_issue_1519
Do some variable name cleanup in lift-json.

We had a memo named declaredFields and another named declaredFieldsMap,
that was added recently to prevent us from having to reflect on each retrieval of
the declared fields. These names were a bit confusing, so we changed them up.

@Shadowfiend Shadowfiend merged commit 78546a0 into master Mar 30, 2014

@Shadowfiend Shadowfiend deleted the msf_issue_1519 branch Mar 30, 2014

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