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

Completely remove support for specifying getter/setter method name in a property declaration #4699

Closed
nadako opened this issue Dec 3, 2015 · 17 comments
Assignees
Milestone

Comments

@nadako
Copy link
Member

nadako commented Dec 3, 2015

Currently, Haxe supports var a(get_a, set_a) along with a(get,set). I guess it was supported to ease transtion from haXe 2, however it's been years and nowadays it only creates confusion, ambiguity and annoyance/issues when working with untyped AST.

So shouldn't we forbid that? I understand that this is a breaking change, so we may want to output a warning for one or two releases and then change it into a proper error.

@ncannasse
Copy link
Member

Could you explain how it causes problem for you?

@Simn
Copy link
Member

Simn commented Dec 3, 2015

It's annoying for refactoring.

@nadako
Copy link
Member Author

nadako commented Dec 3, 2015

Well, the reason I created this issue is that I just had to explain to people that get_<propname> syntax is a leftover from Haxe 2 days, and they can't write var a(getSomeA,...), also I believe if someone wants to write a macro that works with haxe.macro.FieldType.FProp, he has to consider both "get" and "get_" + field.name which is just an annoyance for no obvious reason.

@Simn Simn modified the milestone: 3.4 Feb 23, 2016
@Simn Simn modified the milestones: 3.4, 4.0 Jan 9, 2017
@Simn Simn self-assigned this Sep 22, 2017
@Simn
Copy link
Member

Simn commented Sep 22, 2017

Let's deprecate this for Haxe 4 and remove for 4.1.

@RealyUniqueName
Copy link
Member

Isn't it already deprecated since Haxe 3?

@Simn Simn modified the milestones: Release 4.0, Backlog Apr 17, 2018
@Simn
Copy link
Member

Simn commented Apr 19, 2018

It didn't print deprecation warnings in Haxe 3. But I'm actually willing to just break it for Haxe 4 with an appropriate error message.

@ncannasse
Copy link
Member

ncannasse commented Apr 19, 2018 via email

@mastef
Copy link
Contributor

mastef commented Nov 3, 2018

I don't think it's cool to outright break it. I know of projects that use this, and we use it also in multiple projects. Depreciate first with warning messages... allow people for a transition period instead of breaking everything at once.

@mastef
Copy link
Contributor

mastef commented Nov 3, 2018

And/or: provide macro that would re-enable this feature to work for abandoned legacy projects other projects rely on...

I don't see how it's necessary to add such a major breaking change so rapidly. There's no benefit except adding more workload on people working with the latest haxe version for no reason.

@SavedByZero
Copy link

I agree, I am NOT a fan of this change. I use a proprietary engine that just upgraded its haxe version from 3.4 to 4.1, and now my code is completely broken because of this.

@nadako
Copy link
Member Author

nadako commented Dec 22, 2020

The fix should be just a simple regex find/replace though, isn't it?

@mastef
Copy link
Contributor

mastef commented Dec 23, 2020

The fix should be just a simple regex find/replace though, isn't it?

That's never the case. Then you have dependent libraries, etc.

@SavedByZero
Copy link

SavedByZero commented Dec 23, 2020

Not with hundreds of functions with unique names of get/set_{whatever}. And not when the project is in a code locked, data-updates-only phase (which still sometimes requires compilation for app store versions). Doing such a sweeping find/replace would be extremely risky, however theoretically sound it seems.

@Simn
Copy link
Member

Simn commented Dec 23, 2020

You're transitioning across a major compiler version in a code locked project?

I acknowledge that it was a bit sloppy to not have deprecation warnings in Haxe 3, but it's not like this would have changed much because you still would have to make the changes the same way. Also, there's no point in complaining about this now because we're not going to bring the old syntax back.

Maybe we can provide a regular expression for this. Haxe's property syntax is so unique that it should be quite straightforward and avoid any false positives.

@SavedByZero
Copy link

SavedByZero commented Dec 23, 2020 via email

@nadako
Copy link
Member Author

nadako commented Dec 23, 2020

Something like this maybe?

(var\s+(\w+)\s*\(\s*)(?:(get)_\2|(\w+))(\s*,\s*)(?:(set)_\2|(\w+))(\s*\))

replace string:

$1$3$4$5$6$7$8

UTbIercYcE

@back2dos
Copy link
Member

Here's a build macro to get custom accessors to compile with deprecation warnings:

import haxe.macro.*;

class Haxe3Properties {
  static function support() {
    var ret = Context.getBuildFields(),
        found = false;

    for (f in ret)
      switch f.kind {
        case FProp(get, set, t, e):
          var oldGet = get == 'get_${f.name}',
              oldSet = set == 'set_${f.name}';

          var old = oldGet || oldSet;
          if (old) {
            found = true;
            var used = [],
                expected = [];
            if (oldGet) {
              used.push(get);
              expected.push(get = 'get');
            }
            if (oldSet) {
              used.push(set);
              expected.push(set = 'set');
            }
            Context.warning('Using ${used.join(" and ")} is no longer supported. Use ${expected.join(" and ")} instead.', f.pos);
            f.kind = FProp(get, set, t, e);
          }
        default:
      }

    return
      if (found) ret;
      else null;
  }
}

You can apply this to a whole build with --macro addGlobalMetadata('','@:build(Haxe3Properties.support())') - or better yet only apply it to packages that really need it, to reduce impact on compilation speed).

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

No branches or pull requests

7 participants