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

✨Better Code Organization #225

Closed
wants to merge 23 commits into from
Closed

✨Better Code Organization #225

wants to merge 23 commits into from

Conversation

WillXuCodes
Copy link
Member

@WillXuCodes WillXuCodes commented Jul 15, 2020

Summary:

  • ADI classes were given their own adi namespace under pros, and backwards compatibility was added with the old API with a deprecated warning.
  • Controller and new devices were added to a v5 namespace (under pros namespace).
  • mutex and Task were given their own rtos namespace (also under pros).
  • rtos and v5 namespaces are inline so they already have backwards compatibility.

Motivation:

For better organization if we're to move to doxygen for our docs.

References (optional):

#223 #224

Test Plan:

(This code builds properly from my deviceless testing)

  • Try initiating different pros v5 devices, motors, and adi sensors/motors to see if they work and scope properly with the new code organization.

@WillXuCodes WillXuCodes linked an issue Jul 15, 2020 that may be closed by this pull request
increasing the max size of the controller display.
@WillXuCodes WillXuCodes added p: normal Normal priority rfc This describes a feature, enhancement, or optimization in broad terms and should be discussed labels Jul 15, 2020
@WillXuCodes WillXuCodes requested a review from nathan-moore July 16, 2020 03:37
@WillXuCodes
Copy link
Member Author

@kunwarsahni01 you mind testing this when u got the time? Thanks.

@nathan-moore nathan-moore removed their assignment Jul 16, 2020
@kunwarsahni01
Copy link
Member

Everything seems to be working here, tested with Controller and IMU.
image

@Octogonapus
Copy link
Contributor

This is supposed to be fully backward compatible, right? If so, I would appreciate the chance to test it on OkapiLib.

@WillXuCodes
Copy link
Member Author

This is supposed to be fully backward compatible, right? If so, I would appreciate the chance to test it on OkapiLib.

Yes, everything should be backwards compatible. The v5 devices use an inline namespace to do so, and the ADI stuff uses a macro that also prints a deprecated warning if the old api is used.

@edjubuh
Copy link
Member

edjubuh commented Jul 16, 2020

prints a deprecated warning if the old api is used.

When will the old API be removed? Why is the old API being removed? These explanations would be good to give to users when breaking their code

Octogonapus
Octogonapus previously approved these changes Jul 16, 2020
Copy link
Contributor

@Octogonapus Octogonapus left a comment

Choose a reason for hiding this comment

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

This does not break OkapiLib.

@WillXuCodes
Copy link
Member Author

prints a deprecated warning if the old api is used.

When will the old API be removed? Why is the old API being removed? These explanations would be good to give to users when breaking their code

As for the why, it's to do with us attempting to switch to doxygen for our documentation. This change isn't going to break their code due to the backwards compatibility measures in place, unless you're referring to when we actually remove it down the line.

@edjubuh
Copy link
Member

edjubuh commented Jul 17, 2020

This change isn't going to break their code due to the backwards compatibility measures in place, unless you're referring to when we actually remove it down the line.

If you're putting a warning on their code, it's breaking their code 😃

@HotelCalifornia
Copy link
Contributor

This change isn't going to break their code due to the backwards compatibility measures in place, unless you're referring to when we actually remove it down the line.

If you're putting a warning on their code, it's breaking their code 😃

so this is where the discussion of bumping major versions comes into play. the goal i think is that we'll try to maintain backwards compat (i.e. the code users have works albeit with warnings) for the short term but since it's a major version upgrade there are no guarantees

@edjubuh
Copy link
Member

edjubuh commented Jul 17, 2020

As for the why, it's to do with us attempting to switch to doxygen for our documentation.

I didn't really get the sense that the maintaining backwards compatibility with the current API has a significant impact on improving documentation. I think I'm missing something here.

It's worth mentioning that PROS 2 projects still today should compile in PROS 3. We have a header which #defines all the PROS 2 APIs into PROS 3 functions.

@nickmertin
Copy link
Contributor

so this is where the discussion of bumping major versions comes into play. the goal i think is that we'll try to maintain backwards compat (i.e. the code users have works albeit with warnings) for the short term but since it's a major version upgrade there are no guarantees

It's worth mentioning that PROS 2 projects still today should compile in PROS 3. We have a header which #defines all the PROS 2 APIs into PROS 3 functions.

So perhaps, keep the backwards-compatibility for a PROS 3 release, then break compatibility with PROS 4? Otherwise, we run into the issue of having some things break on 4.0, while others break on 4.1, etc. IMO breaking everything that we plan to break in one release is better because it limits the number of releases in which people have to worry about breaking changes, and in the long run doesn't really affect the compatibility. Thoughts?

@HotelCalifornia
Copy link
Contributor

IMO this discussion has gone a little bit off the rails. the specific purpose of this PR is to better organize PROS classes into logical units. deprecating the old ADI class names is, however disruptive to users' workflow, not a breaking change in my mind. code will still compile, and those who choose to ignore the warnings may do so at their own risk. beyond that, there will be no tangible change to the end-user API, except that some classes may, if the user so chooses, be referred to by more fully-qualified names.

if and when version 4 lands, we can certainly create another "legacy header" for PROS 3 in the same way that one was created for PROS 2. however, that change specifically is out of the scope of this PR.

@WillXuCodes WillXuCodes changed the title ✨Better Code Organization ✨Better Code Organization (Revisit when PROS 4 work continues) Aug 9, 2020
@WillXuCodes WillXuCodes changed the title ✨Better Code Organization (Revisit when PROS 4 work continues) ✨Better Code Organization (Merge alongside PROS 4 merges) Aug 9, 2020
@HotelCalifornia
Copy link
Contributor

that's why I converted it into a draft PR... no need to close it

@HotelCalifornia HotelCalifornia changed the title ✨Better Code Organization (Merge alongside PROS 4 merges) ✨Better Code Organization Oct 28, 2020
@HotelCalifornia HotelCalifornia added this to the 4.0.0 milestone Oct 28, 2020
@WillXuCodes WillXuCodes added on hold This may be revisited in the future enhancement This builds on top of an existing feature and removed p: normal Normal priority rfc This describes a feature, enhancement, or optimization in broad terms and should be discussed labels Jan 24, 2021
@WillXuCodes
Copy link
Member Author

WillXuCodes commented Jun 7, 2021

Fixed up this PR after merges with the ext-adi changes, not much other than renaming and fixing up a couple issues with merge conflicts. Needs re-testing before PROS-4 merge.

@WillXuCodes WillXuCodes assigned WillXuCodes and unassigned ayushuk Mar 7, 2022
@WillXuCodes
Copy link
Member Author

Closing since all work for PROS 4 will be done on the develop-pros-4 branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This builds on top of an existing feature on hold This may be revisited in the future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PROS 4: Better Namespace Layout
8 participants