Skip to content

Conversation

@jsoref
Copy link
Contributor

@jsoref jsoref commented Dec 31, 2018

Generated by https://github.com/jsoref/spelling f; to maintain your repo, please consider fchurn

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
    • I haven't opened an issue. I can at some point. I don't expect it to be particularly helpful. It took a couple of weekends to put together this PR...
    • These were rebased to master (because there were conflicts). This will fall behind, fchurn can be used to deal w/ rebasing the last mile if people are interested.
    • These aren't squashed. There are a number of changes that need to be considered, and it's much easier for me if people review the delta and complain about word families. I'll drop individual families as I receive objections. Once objections are handled, squashing is trivial.
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS that we support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm hoping this is a debugging object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is just a section name, but it's possible things link to it

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be high, rather than height

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is just a section name, but it's possible things link to it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is just a section name, but it's possible things link to it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is just a section name, but it's possible things link to it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably an api change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really not sure about this

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

I tried to do a first pass on this, but really I think it is too big for us to ever actually get this merged.
I think I got about 2/3 of the way through it, and that was just glancing at each change looking for obvious problems.

Just doing to proper review of each change will take a couple of hours, and that's not accounting for looking into the changes that are questionable (many of which you've already highlighted).

By the time we reached a conclusion on all the changes, the code base would have moved so much that the merges would become ugly and someone would have to spend more time sorting that out.

If you want to see this to completion I think we need to get it split into manageable pieces. My recommendations:

  • Start with anything in the docs because those have real value (and there's probably not many of them).
  • Skip the ie. -> i.e. and eg. -> e.g. ; Yes it's technically correct, but there's a lot of them and it creates a lot of noise for very minimal value.
  • Split the API changes (class names, public method names) into separate PRs so that they can get 1 good review and be merged. e.g. one for each of the isCacheable and Interruptible changes.
  • then let's see what's left over.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be high, rather than height

Copy link
Contributor

Choose a reason for hiding this comment

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

The page it is linking to is https://www.elastic.co/guide/en/elasticsearch/guide/current/pluggable-similarites.html (which has a typo, but we can live with it).

Can you revert this change please?


/**
* @deprecated new implementors should override {@link #read(StreamInput)} and use the
* @deprecated new implementers should override {@link #read(StreamInput)} and use the
Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking both implementor and implementer are correct usage, so I'm not sure it's worth the churn to change these.

throw new IllegalArgumentException("final " + type + " setting [" + key + "], not updatable");
} else {
throw new IllegalArgumentException(type + " setting [" + key + "], not dynamically updateable");
throw new IllegalArgumentException(type + " setting [" + key + "], not dynamically updatable");
Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly, both are correct, but our docs seem to prefer updatable, so this is probably a good change.

@@ -277,14 +277,14 @@ public void testDefault() {

// It gets more complicated when there are two settings objects....
Settings hasFallback = Settings.builder().put("foo.bar", "o").build();
Setting<String> fallsback =
Setting<String> fallsBack =
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to change this one, I think it should just be fallback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I normally would have picked fallback, the only reason I didn't was that I was wondering if the code was trying to speak about the action of falling back as opposed to the actual fallback.

I'm quite happy to switch (and will when I get to the code side, I think that'll come after the API bits get split out).

@jsoref
Copy link
Contributor Author

jsoref commented Dec 31, 2018

Sure.

@javanna
Copy link
Member

javanna commented Dec 31, 2018

I agree with @tvernum , these are good changes, but they should be made one step at a time in smaller PRs. I will close this then, looking forward to your upcoming PRs @jsoref !

@jsoref
Copy link
Contributor Author

jsoref commented Dec 31, 2018

For reference,
https://github.com/jsoref/elasticsearch/tree/spelling is more or less "what's left" and
https://github.com/jsoref/elasticsearch/tree/spelling-full is more or less the full set of changes (including i.e./e.g. and similarities, but with the correction for high, i.e. roughly the original PR)

@jsoref
Copy link
Contributor Author

jsoref commented Dec 31, 2018

I think this is as far as I'm likely to go today.

@javanna
Copy link
Member

javanna commented Dec 31, 2018

thanks you @jsoref !

@jsoref
Copy link
Contributor Author

jsoref commented Dec 31, 2018

For my reference, the elasticsearch build system doesn't like my git metadata... The following works to strip the metadata (by recreating a branch's single commit w/ its single-line commit message):

for a in $(cat); do
git checkout master;
git branch -D X;
git checkout -b X;
git show $a |patch -p1;
git add .;
git commit -m "$(git log --format=%B -n 1 $a|head -1)";
git diff $a..X &&
 git branch -D $a &&
 git checkout -b $a &&
 git branch -D X &&
 git push github $a -f;
done

(yes, one of the two deletes of the X branch will fail, but that's just paranoia)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants