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

[Not Urgent, Stylistic] Rename length to len. #182

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Paalon
Copy link
Contributor

@Paalon Paalon commented Jun 14, 2024

Julia Base exports the name length thus rename for disambiguation.

Copy link
Collaborator

@kescobo kescobo left a comment

Choose a reason for hiding this comment

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

I'd marginally prefer something more descriptive rather than less - like out_length or str_length or something.

Here, where it's just a lot of find-replace, and it's going to be read a lot more than it's written, trying to be economical at the expense of readability strikes me as a mistake

@Paalon
Copy link
Contributor Author

Paalon commented Jun 16, 2024

I read src/buffer_input.jl and src/scanner.jl many times. A few days after sending this pull request, I noticed a few things. The variable name length is used as the 2 following meanings:

  1. the position of the current peeking character (0-based)
  2. the length of the processed token

In Julia, I think this variable should mean 1-based position, and the name should be pos and len = pos - 1. The name position is also exported by Julia base thus pos will be fit. If longer name is desired, character_position may be good but I don't think the longer name is required because it is obvious that we are processing a stream and what pos or len means.

Anyway, this PR is not urgent. It's okay to think about this later.

@Paalon Paalon changed the title Rename length to len. [Not Urgent, Stylistic] Rename length to len. Jun 16, 2024
@kescobo
Copy link
Collaborator

kescobo commented Jun 16, 2024

I don't think the longer name is required because it is obvious

I always worry about statements like this - I have on many occasions thought "this will be obvious to future me", and then come back after a couple of months it more and have no idea what's going on.

You're probably right that pos and len are obvious, but even something like charpos is a bit more explicit, and it would make me feel better 😉 .

@GunnarFarneback
Copy link
Contributor

In Julia, I think this variable should mean 1-based position

For what it's worth, position, seek and other stream operations in Julia follow the convention to consider the position as a 0-based offset from the start of the file.

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

Successfully merging this pull request may close these issues.

3 participants