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.

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

C++: Remove getLocation from Container. #14337

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aschackmull
Copy link
Contributor

Follow up to #14321

With the above-mentioned PR, Container now has conflicting urls provided by getURL and getLocation, respectively. This PR removes getLocation, since getURL ought to be sufficient. I was a bit in doubt about whether to switch the extends to Element or ElementBase, but it seemed that the additional predicates inherited from Element made little sense for Containers, so I chose ElementBase.

cc @jketema

@aschackmull aschackmull requested a review from a team as a code owner September 28, 2023 13:13
@github-actions github-actions bot added the C++ label Sep 28, 2023
@jketema
Copy link
Contributor

jketema commented Sep 28, 2023

Is it actually safe to just remove these, or do we need to go through a deprecation period?

@aschackmull
Copy link
Contributor Author

Is it actually safe to just remove these, or do we need to go through a deprecation period?

Good question. I just wanted to put up the diff for suggestion. The PR actually removes a whole bunch of other predicates as well that were previously inherited from Element - changing the type hierarchy like this is always tricky in terms of potential breakage. Maybe all of the removed predicates should be re-added as deprecated, but no longer inherited nor overriding.

@aschackmull
Copy link
Contributor Author

It's certainly breaking some tests. Do you perhaps want to take it from here? I don't feel strongly enough about this change to spend a lot of time on it.

@jketema
Copy link
Contributor

jketema commented Sep 28, 2023

It's certainly breaking some tests. Do you perhaps want to take it from here? I don't feel strongly enough about this change to spend a lot of time on it.

I'll have a closer look.

@aschackmull
Copy link
Contributor Author

It's also perfectly fine to re-add

Location getLocation() {
    result.getContainer() = this and
    result.hasLocationInfo(_, 0, 0, 0, 0)
  }

on File without extending Locatable. A class doesn't need to extend Locatable to be able to provide a getLocation predicate.

@jketema
Copy link
Contributor

jketema commented Sep 28, 2023

It's certainly breaking some tests.

I think it's breaking almost all the tests.

@jketema
Copy link
Contributor

jketema commented Sep 29, 2023

I've been looking at this a bit. I think File will still need to extend Element. Not having this just breaks too much code (code I'm not really willing to touch) and that code will need special cases for Files if I do touch it. There might still be consequences of Folder not having a getLocation, but those somewhat hidden for me due to all test regressions related to File not extending Element.

@aschackmull
Copy link
Contributor Author

I think File will still need to extend Element.

Sounds fine. In that case it might as well extend Locatable, since that doesn't add any member predicates, but merely indicates that getLocation can be expected to give something reasonable (which it does).

@jketema
Copy link
Contributor

jketema commented Sep 29, 2023

I think File will still need to extend Element.

Sounds fine. In that case it might as well extend Locatable, since that doesn't add any member predicates, but merely indicates that getLocation can be expected to give something reasonable (which it does).

This fixes all the tests (see the internal PR that now links to this one).

Given the nature of this change, I'm tempted to make this one breaking instead of having a deprecation period. However, I'll need to check with some people that this reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants