Yeah. I think it's worth even if it's a bit of boilerplate code. I think we might be a bit more friendly and welcoming to new/accidental users who just want to make a small PR and are not regular users. Those people will often use IDEs (and Pycharm is by far the most popular in python world). Similarly to AIP-7 I think if those people have lower barrier entry and better guidance, we will only all benefit from that :).
J. On Tue, Jun 11, 2019 at 8:53 PM Kamil Breguła <[email protected]> wrote: > If I added a warning in the method that it calls immediately, IDE > still could not detect everything correctly. The user is not able to > see the error message in the IDE. The warning appears in the wrong > place. > https://i.imgur.com/CSqP1wU.png > > Adding a new import allows you to avoid some IDE warnings, but the > problem remains that this module is not detected as deprecated > https://i.imgur.com/CSqP1wU.png > . > > On Thu, Jun 6, 2019 at 11:32 AM Ash Berlin-Taylor <[email protected]> wrote: > > > > I hate IDEs :( > > > > What does the IDE show if you package up the deprecation warning in some > way in your solution (i.e. the warning.warn isn't directly in the module > source)? Alternatively adding `from solution2.new import *` into > solution2.deprecated may help the IDE? > > > > -a > > > > > On 5 Jun 2019, at 17:41, Kamil Breguła <[email protected]> > wrote: > > > > > > I have done a deep investigation of this issue. Please, look at the > > > beginning of chapter "Case #7". > > > > > > I prepared a repository with simple demo and also shared a screenshot > from > > > my IDE. > > > Photo: https://imgur.com/a/mRaWpQm > > > Repo: https://github.com/mik-laj/airflow-deprecation-sample > > > Today I looked again at this project. I recently updated the IDE to the > > > latest version - IntelliJ IDEA 2019.1.2 (Ultimate Edition). The issue > still > > > exists. > > > I made additional photos today: https://imgur.com/a/4fcmkVX > > > > > > IDE uses statistical analysis of the code and gets lost in external > > > libraries. > > > > > > If you have questions, I will be happy to give you further > explanations. > > > > > > On Wed, Jun 5, 2019 at 4:30 PM Ash Berlin-Taylor <[email protected]> > wrote: > > > > > >>> Case 7: Zope deprecation or not > > >>> *B*: I prefer to use Kamil's in-house hack than zope, but I feel we > could > > >>> improve it slightly to remove a bit of boilerplate - it looks like > the > > >>> warning.warn() message should be very similar in all deprecation > cases > > >> and > > >>> possibly we can define it in one place (providing that > DepracationWarning > > >>> will continue to work well with IDEs) > > >> > > >> At this point wouldn't or home-rolled solution be very similar to > > >> zope.deprecation (which is already a dep of Airflow)? > > >> > > >> Also are we sure that zope.deprecation isn't supported by an IDE? It > still > > >> issues a warning.warn. (The example docs in zope.deprecation show > messing > > >> about with sys.modules but that isn't needed.) > > >> > > >> https://gist.github.com/ashb/327a79d7c6c1f5a2344cd1812a8ee92d shows a > > >> formatted example: > > >> > > >> ``` > > >> #Place this as airflow/foo.py > > >> import zope.deprecation > > >> zope.deprecation.moved('airflow.settings', 'version 2') > > >> ``` > > >> > > >> And then when you import that module you get: > > >> > > >> ``` > > >> /Users/ash/.virtualenvs/airflow/bin/ipython:1: DeprecationWarning: > > >> airflow.foo has moved to airflow.settings. Import of airflow.foo will > > >> become unsupported in version 2 > > >> #!/Users/ash/.virtualenvs/airflow/bin/python3.7 > > >> > > >> ``` > > >> > > >> That should work fine with IDE? > > >> > > >> > > >>> On 5 Jun 2019, at 14:56, Jarek Potiuk <[email protected]> > wrote: > > >>> > > >>> I think it's very important to discuss this and choose the right > approach > > >>> and follow up quickly. This is very important to introduce some > sanity in > > >>> the structure of 2.0.0. > > >>> I propose that anyone with a strong opinion voices his or her > concerns > > >> here > > >>> and then maybe a week from now we can get to a consensus and vote on > the > > >>> direction we take? > > >>> > > >>> My preferences (but I can be convinced to change some of my choices) > are: > > >>> > > >>> Case 1: (contrib vs not) > > >>> *B* (move all that are "well" tested): I think having "incubator" > kind of > > >>> approach for not super tested/documented but still usable code is > useful. > > >>> Though we would have to establish some objective criteria and > describe a > > >>> "growing from incubation" process. > > >>> > > >>> Case 2 airflow.operators.foo_operator.FooOperator could become > > >>> *A:* airflow.operators.foo.FooOperator > > >>> > > >>> Case 3: > airflow.contrib.operators.gcp_bigtable_operator.BigTableOperator > > >>> could become > > >>> *C*: airflow.integration.gcp.operators.bigtable.BigTableOperator > > >>> > > >>> Case 4: (separate packages/namesapces) > > >>> *B:* no namespaces - i think we are very far from modularisation to > > >> happen > > >>> (and how) and it might get never implemented so there are no real > > >> benefits > > >>> I see now. > > >>> > > >>> Case 5: "Operator" suffix on class names > > >>> *B*: I prefer to keep B as this will be much easier when you search > for > > >>> class using your IDE. > > >>> > > >>> Case 6: "Isolated" renames > > >>> *Rename*: We should align names of those operators. > > >>> > > >>> Case 7: Zope deprecation or not > > >>> *B*: I prefer to use Kamil's in-house hack than zope, but I feel we > could > > >>> improve it slightly to remove a bit of boilerplate - it looks like > the > > >>> warning.warn() message should be very similar in all deprecation > cases > > >> and > > >>> possibly we can define it in one place (providing that > DepracationWarning > > >>> will continue to work well with IDEs) > > >>> > > >>> J, > > >>> > > >>> On Tue, Jun 4, 2019 at 2:11 PM Kamil Breguła < > [email protected]> > > >>> wrote: > > >>> > > >>>> I created AIP-21 > > >>>> > > >>>> > > >> > https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-21%3A+Changes+in+import+paths > > >>>> This is the same document as above, I just deleted my opinions > (summary > > >>>> sections) and work plan. > > >>>> > > >>>> > > >>>> On Fri, Apr 19, 2019 at 2:18 PM Jarek Potiuk < > [email protected]> > > >>>> wrote: > > >>>> > > >>>>> Thanks Kamil! Excellent analysis! > > >>>>> > > >>>>> My preferences: > > >>>>> > > >>>>> Case 1: (contrib vs not) > > >>>>> B (move all that are "well" tested): I think having "incubator" > kind of > > >>>>> approach for not super tested/documented but still usable code is > > >> useful. > > >>>>> Though we would have to establish some objective criteria and > describe > > >> a > > >>>>> "growing from incubation" process. > > >>>>> > > >>>>> Case 2 airflow.operators.foo_operator.FooOperator could become > > >>>>> A: airflow.operators.foo.FooOperator > > >>>>> > > >>>>> Case 3: > > >> airflow.contrib.operators.gcp_bigtable_operator.BigTableOperator > > >>>>> could become > > >>>>> C: airflow.integration.gcp.operators.bigtable.BigTableOperator > > >>>>> > > >>>>> Case 4: (separate packages/namesapces) > > >>>>> B: no namespaces - i think we are very far from modularisation to > > >> happen > > >>>>> (and how) and it might get never implemented so there are no real > > >>>> benefits > > >>>>> I see now. > > >>>>> > > >>>>> Case 5: "Operator" suffix on class names > > >>>>> B: I prefer to keep B as this will be much easier when you search > for > > >>>> class > > >>>>> using your IDE. > > >>>>> > > >>>>> Case 7: Zope deprecation or not > > >>>>> B: I prefer to use Kamil's in-house hack than zope, but I feel we > could > > >>>>> improve it slightly to remove a bit of boilerplate - it looks like > the > > >>>>> warning.warn() message should be very similar in all deprecation > cases > > >>>> and > > >>>>> possibly we can define it in one place (providing that > > >> DepracationWarning > > >>>>> will continue to work well with IDEs) > > >>>>> > > >>>>> > > >>>>> > > >>>>> > > >>>>> On Fri, Apr 19, 2019 at 12:36 AM Kamil Breguła < > > >>>> [email protected]> > > >>>>> wrote: > > >>>>> > > >>>>>> Inline comment > > >>>>>> > > >>>>>> On Fri, Apr 19, 2019 at 12:23 AM Arthur Wiedmer < > > >>>>> [email protected]> > > >>>>>> wrote: > > >>>>>> > > >>>>>>> > > >>>>>>>>>>> Case #5 Are we talking about the class name or the file name? > > >>>>> The > > >>>>>>>> class > > >>>>>>>>>>> name is fine to me, but we could remove _operator from the > > >>>> file > > >>>>>>> name. > > >>>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>> Case #2 describes the change of file names > > >>>>>>>>>> Case #5 describes the change of class names > > >>>>>>>>>> > > >>>>>>>>>> I added examples to two cases to better describe the changes. > > >>>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> Thank you. I guess I see it for file names, but I am wondering > > >>>>> about > > >>>>>>> the > > >>>>>>>>> operators and name collisions. > > >>>>>>>>> Say I need both the HiveOperator and I inline a custom operator > > >>>> for > > >>>>>>>> which I > > >>>>>>>>> need a hive hook. > > >>>>>>>>> # Would you recommmend the following? Or does case #5 only > apply > > >>>> to > > >>>>>>>>> Operators? > > >>>>>>>>> from airflow.operators.hive import Hive as HiveOperator > > >>>>>>>>> from airfow.hooks.hive import Hive as HiveHook > > >>>>>>>>> > > >>>>>>>>> I was just thinking it might be nice to be able to import what > I > > >>>>> need > > >>>>>>>>> without renaming and not worry too much about names shadowing > > >>>>> others. > > >>>>>>>>> Especially if I am new-ish to Apache Airflow. > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>> I think that the operator should describe the behavior(verb), > hook > > >>>>>> should > > >>>>>>>> describe a service(noun). These are other parts of speech. > > >>>>>>>> In my opinion, HiveOperator should be named HiveExecuteQuery. > Hook > > >>>>> will > > >>>>>>> be > > >>>>>>>> called Hive. Then there will be no name conflicts. > > >>>>>>>> > > >>>>>>> > > >>>>>>> But we will still provide the backwards compatibility for a while > > >>>> with > > >>>>>>> aliases to the old names, correct? > > >>>>>>> > > >>>>>> > > >>>>>> Yes. It is possible to provide backward compatibility also in this > > >>>> case. > > >>>>>> > > >>>>>> > > >>>>>> -- > > >>>>>> > > >>>>>> Kamil Breguła > > >>>>>> Polidea <https://www.polidea.com/> | Software Engineer > > >>>>>> > > >>>>>> M: +48 505 458 451 <+48505458451> > > >>>>>> E: [email protected] > > >>>>>> [image: Polidea] <https://www.polidea.com/> > > >>>>>> > > >>>>>> We create human & business stories through technology. > > >>>>>> Check out our projects! <https://www.polidea.com/our-work> > > >>>>>> [image: Github] <https://github.com/Polidea> [image: Facebook] > > >>>>>> <https://www.facebook.com/Polidea.Software> [image: Twitter] > > >>>>>> <https://twitter.com/polidea> [image: Linkedin] > > >>>>>> <https://www.linkedin.com/company/polidea> [image: Instagram] > > >>>>>> <https://instagram.com/polidea> [image: Behance] > > >>>>>> <https://www.behance.net/polidea> > > >>>>>> > > >>>>> > > >>>>> > > >>>>> -- > > >>>>> > > >>>>> Jarek Potiuk > > >>>>> Polidea <https://www.polidea.com/> | Principal Software Engineer > > >>>>> > > >>>>> M: +48 660 796 129 <+48660796129> > > >>>>> E: [email protected] > > >>>>> > > >>>> > > >>>> > > >>>> -- > > >>>> > > >>>> Kamil Breguła > > >>>> Polidea <https://www.polidea.com/> | Software Engineer > > >>>> > > >>>> M: +48 505 458 451 <+48505458451> > > >>>> E: [email protected] > > >>>> [image: Polidea] <https://www.polidea.com/> > > >>>> > > >>>> We create human & business stories through technology. > > >>>> Check out our projects! <https://www.polidea.com/our-work> > > >>>> [image: Github] <https://github.com/Polidea> [image: Facebook] > > >>>> <https://www.facebook.com/Polidea.Software> [image: Twitter] > > >>>> <https://twitter.com/polidea> [image: Linkedin] > > >>>> <https://www.linkedin.com/company/polidea> [image: Instagram] > > >>>> <https://instagram.com/polidea> [image: Behance] > > >>>> <https://www.behance.net/polidea> > > >>>> > > >>> > > >>> > > >>> -- > > >>> > > >>> Jarek Potiuk > > >>> Polidea <https://www.polidea.com/> | Principal Software Engineer > > >>> > > >>> M: +48 660 796 129 <+48660796129> > > >>> E: [email protected] > > >> > > >> > > > > > > -- > > > > > > Kamil Breguła > > > Polidea <https://www.polidea.com/> | Software Engineer > > > > > > M: +48 505 458 451 <+48505458451> > > > E: [email protected] > > > [image: Polidea] <https://www.polidea.com/> > > > > > > We create human & business stories through technology. > > > Check out our projects! <https://www.polidea.com/our-work> > > > [image: Github] <https://github.com/Polidea> [image: Facebook] > > > <https://www.facebook.com/Polidea.Software> [image: Twitter] > > > <https://twitter.com/polidea> [image: Linkedin] > > > <https://www.linkedin.com/company/polidea> [image: Instagram] > > > <https://instagram.com/polidea> [image: Behance] > > > <https://www.behance.net/polidea> > > > -- Jarek Potiuk Polidea <https://www.polidea.com/> | Principal Software Engineer M: +48 660 796 129 <+48660796129> [image: Polidea] <https://www.polidea.com/>
