Remove unnecessary `FileDescriptor` overrides by bdmendes · Pull Request #1129 · adzerk/apso

bdmendes · 2026-07-20T15:50:35Z

These operations are safe to perform at the base. There is no need to repeat them in implementations – they were sometimes slightly more efficient (e.g. not allocating intermediate wrappers while descending through child directories) but I don't think that is significant enough to maintain more code.

Does this change relate to existing issues or pull requests?

Opportunistic change after #1128.

Does this change require an update to the documentation?

No.

How has this been tested?

I am relying on the soundness of the parent and child implementations.

jcazevedo

Thanks for this!

I think this change makes sense, but I would still keep open the opportunity for implementations to override specific methods (i.e. remove the finals), particularly with FileDescriptor not being sealed.

jcazevedo · 2026-07-22T13:49:16Z

    *   an iterator with the lines of this file.
    */
-  def lines(): Iterator[String] = Source.fromInputStream(stream()).getLines()
+  final def lines(): Iterator[String] = Source.fromInputStream(stream()).getLines()


I think it's still reasonable to give the option of implementations to redefine this, no?

Suggested change

final def lines(): Iterator[String] = Source.fromInputStream(stream()).getLines()

def lines(): Iterator[String] = Source.fromInputStream(stream()).getLines()

Please see 19f3ab3.

jcazevedo · 2026-07-22T13:49:30Z

    *   the new file descriptor with the updated path
    */
-  def /(name: String): Self = child(name)
+  final def /(name: String): Self = child(name)


I think it's still reasonable to give the option of implementations to redefine this, no?

Suggested change

final def /(name: String): Self = child(name)

def /(name: String): Self = child(name)

jcazevedo · 2026-07-22T13:49:41Z

    *   the new file descriptor with the updated path
    */
-  def children(names: String*): Self =
+  final def children(names: String*): Self =


I think it's still reasonable to give the option of implementations to redefine this, no?

Suggested change

final def children(names: String*): Self =

def children(names: String*): Self =

Please see 1549f5e.

jcazevedo · 2026-07-22T13:49:59Z

    *   the new file descriptor with the updated path
    */
-  def cd(pathString: String): Self = {
+  final def cd(pathString: String): Self = {


I think it's still reasonable to give the option of implementations to redefine this, no?

Suggested change

final def cd(pathString: String): Self = {

def cd(pathString: String): Self = {

Please see d5d001e.

jcazevedo · 2026-07-22T13:50:10Z

    *   a new file descriptor pointing to a sibling of the current file descriptor
    */
-  def sibling(name: String): Self = sibling(_ => name)
+  final def sibling(name: String): Self = sibling(_ => name)


I think it's still reasonable to give the option of implementations to redefine this, no?

Suggested change

final def sibling(name: String): Self = sibling(_ => name)

def sibling(name: String): Self = sibling(_ => name)

Please see 1549f5e.

Please see 1549f5e.

I don't think this commit is removing the final keyword. Intended?

Meant to comment on the inner sibling. I'll argue for keeping the final for the adapter methods next.

jcazevedo · 2026-07-22T13:50:20Z

    *   a new file descriptor pointing to a sibling of the current file descriptor
    */
-  def sibling(f: String => String): Self = parent().child(f(name)).asInstanceOf[Self]
+  final def sibling(f: String => String): Self = parent().child(f(name)).asInstanceOf[Self]


I think it's still reasonable to give the option of implementations to redefine this, no?

Suggested change

final def sibling(f: String => String): Self = parent().child(f(name)).asInstanceOf[Self]

def sibling(f: String => String): Self = parent().child(f(name)).asInstanceOf[Self]

Please see 19f3ab3.

jcazevedo · 2026-07-22T14:57:11Z

    */
-  def cd(pathString: String): Self = {
+  final def cd(pathString: String): Self = {
    pathString.split("/").toList.foldLeft(this.asInstanceOf[Self]) {


All the implementations of this were trimming the individual splits. I can't recall why. Maybe we were just being overcautious for malformed inputs. That being said, it might not be a bad idea to keep the trimming in the overarching implementation for the time being.

Brought back the trimming in 1549f5e.

jcazevedo · 2026-07-22T14:59:36Z

  protected def duplicate(elements: List[String]): Self

-  def parent(n: Int = 1): Self =
+  final def parent(n: Int = 1): Self =


I think it's still reasonable to give the option of implementations to redefine this, no?

Suggested change

final def parent(n: Int = 1): Self =

def parent(n: Int = 1): Self =

jcazevedo · 2026-07-22T14:59:46Z

    this.duplicate(elements = elements.dropRight(n))

-  def child(name: String): Self =
+  final def child(name: String): Self =


I think it's still reasonable to give the option of implementations to redefine this, no?

Suggested change

final def child(name: String): Self =

def child(name: String): Self =

bdmendes · 2026-07-23T10:43:57Z

I agree that, in some cases, it might make sense to keep the possibility for implementations to override as needed for performance. However, in the cases of / (alias to child) and the sibling adapter I don't think it is relevant. Let me know.

Co-authored-by: João Costa <jdpc557@gmail.com>

jcazevedo · 2026-07-23T10:55:22Z

I agree that, in some cases, it might make sense to keep the possibility for implementations to override as needed for performance. However, in the cases of / (alias to child) and the sibling adapter I don't think it is relevant. Let me know.

Fair enough.

jcazevedo

Thank you!

bdmendes self-assigned this Jul 20, 2026

bdmendes force-pushed the simplify-fds-impls branch 2 times, most recently from e9536ca to c6e905c Compare July 20, 2026 17:00

bdmendes requested a review from jcazevedo July 21, 2026 10:40

bdmendes marked this pull request as ready for review July 21, 2026 10:40

Base automatically changed from sftp-rename to master July 22, 2026 13:31

Remove unnecessary FileDescriptor overrides

97c2124

afonsoafonsoafonso force-pushed the simplify-fds-impls branch from c6e905c to 97c2124 Compare July 22, 2026 13:31

jcazevedo requested changes Jul 22, 2026

View reviewed changes

bdmendes added 2 commits July 23, 2026 10:54

Trim in base cd

1549f5e

Allow redefinition when needed for performance

19f3ab3

bdmendes force-pushed the simplify-fds-impls branch from 3086784 to 19f3ab3 Compare July 23, 2026 09:59

Allow overriding cd

d5d001e

bdmendes requested a review from jcazevedo July 23, 2026 10:44

JD557 reviewed Jul 23, 2026

View reviewed changes

Comment thread modules/io/src/main/scala/com/kevel/apso/io/FileDescriptor.scala Outdated

Avoid allocation

36a7dc4

Co-authored-by: João Costa <jdpc557@gmail.com>

jcazevedo approved these changes Jul 23, 2026

View reviewed changes

Merge branch 'master' into simplify-fds-impls

02e6b1f

jcazevedo merged commit e2dcf8c into master Jul 23, 2026
10 checks passed

jcazevedo deleted the simplify-fds-impls branch July 23, 2026 11:07

	final def lines(): Iterator[String] = Source.fromInputStream(stream()).getLines()
	def lines(): Iterator[String] = Source.fromInputStream(stream()).getLines()

	final def /(name: String): Self = child(name)
	def /(name: String): Self = child(name)

	final def children(names: String*): Self =
	def children(names: String*): Self =

	final def cd(pathString: String): Self = {
	def cd(pathString: String): Self = {

	final def sibling(name: String): Self = sibling(_ => name)
	def sibling(name: String): Self = sibling(_ => name)

	final def sibling(f: String => String): Self = parent().child(f(name)).asInstanceOf[Self]
	def sibling(f: String => String): Self = parent().child(f(name)).asInstanceOf[Self]

	final def parent(n: Int = 1): Self =
	def parent(n: Int = 1): Self =

	final def child(name: String): Self =
	def child(name: String): Self =

Uh oh!

Conversation

bdmendes commented Jul 20, 2026

Does this change relate to existing issues or pull requests?

Does this change require an update to the documentation?

How has this been tested?

Uh oh!

jcazevedo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdmendes commented Jul 23, 2026

Uh oh!

Uh oh!

jcazevedo commented Jul 23, 2026

Uh oh!

jcazevedo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants