Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Replace Deprecated classes like Stack/Stream #1423

Merged

Conversation

olabusayoT
Copy link
Contributor

  • remove TransientParam class, replace with just @transient
  • replace deprecated Stream with LazyList
  • rename iterator to eventIterator since Iterator trait now defines iterator field
  • .toIterator -> .iterator
  • update collection converters for 2.13, add compat class to enable for 2.12
  • remove additional implicitCoversions/reflectiveCalls imports since it causes an unused import error in 2.13
  • replace mutable.ArrayStack/Stack with custom StackLike that is backed by ListBuffer
  • replace deprecated Proxy class with overrides to toString, hashCode and equals
  • non-explicit implicit return types are deprecated in 2.13
  • parseString is required for 2.13 extenders of Numeric, set to None

DAFFODIL-2152

Copy link
Contributor

@mbeckerle mbeckerle left a comment

Choose a reason for hiding this comment

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

+1

Just minor comments/suggestions.

structs.top.initChoiceStatements += s"$indent ${arrayName}_initERD(instance, parent);"
structs
.top()
.initChoiceStatements += s"$indent ${arrayName}_initERD(instance, parent);"
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest just inserting a comment at these statements indicating they are not covered by tests
E.g.,

// TODO: not convered by tests

But don't bother to try to figure them out.

val x = resume(
producer,
dummy
) // producer isn't sent anything. It's just resumed to get another value.
x match {
case EndOfData => Stream.Empty
case EndOfData => LazyList.empty
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment // TODO: no test coverage

@@ -30,16 +30,16 @@ import scala.language.implicitConversions

package object numerics {
implicit def toRicherInt(x: Int): RicherInt = new RicherInt(x)
implicit def toRicherInt(x: scala.runtime.RichInt) = new YetRicherInt(
implicit def toRicherInt(x: scala.runtime.RichInt): YetRicherInt = new YetRicherInt(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest don't bother putting any comments into the passera library code about test coverage that is missing.

* have been deprecated in 2.13. This allows us to maintain the same
* functionality as stack while using ListBuffer instead
*
* ArrayDeque is the recommended replacement but is not available in 2.12
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop comment.

Even if ArrayDeque is recommended, there is value in calling a stack a Stack. I would not recommend removing this even if ArrayDeque has all the functionality, because well, an ArrayDeque presumably also has non-stack additional behavior and if we are only using the Stack behavior then that is worth knowing, preferably by having this class/trait that enforces this.

I mean if I am reading code, and it uses a data structure that uses "double ended queue", then I'm assuming they chose that because they need more than just stack behavior. So it's misleading to me if someone only needs stack behavior, but uses a more general data structure which does not have 'stack' in its name.

* TODO: If 2.12 support is dropped, this class should be deleted and ArrayDeque used
* instead
*/
class StackLike[T] {
Copy link
Contributor

Choose a reason for hiding this comment

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

StackLike sounds like a name of a trait, not a concrete class. Is there a reason this can't just be Stack?

I would even suggest Stack1 is better than StackLike, because Stack1 suggests the name of a concrete instantiable class.

Copy link
Contributor

@jadams-tresys jadams-tresys left a comment

Choose a reason for hiding this comment

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

+1

Agree with Mike about changing StackLike to just Stack.

- remove TransientParam class, replace with just @transient
- replace deprecated Stream with LazyList
- rename iterator to eventIterator since Iterator trait now defines iterator field
- .toIterator -> .iterator
- update collection converters for 2.13, add compat class to enable for 2.12
- remove additional implicitCoversions/reflectiveCalls imports since it causes an unused import error in 2.13
- replace mutable.ArrayStack/Stack with custom Stack class that is backed by ListBuffer
- replace deprecated Proxy class with overrides to toString, hashCode and equals
- non-explicit implicit return types are deprecated in 2.13
- parseString is required for 2.13 extenders of Numeric, set to None
- add comments about code not covered by tests

DAFFODIL-2152
@olabusayoT olabusayoT force-pushed the daf-2152-replace-deprecated-classes branch from 23fccd5 to ccd6f5c Compare February 10, 2025 18:06
@olabusayoT olabusayoT merged commit c9a5f78 into apache:main Feb 10, 2025
11 of 12 checks passed
@olabusayoT olabusayoT deleted the daf-2152-replace-deprecated-classes branch February 10, 2025 18:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants