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

rename drop to drop/zero slice from #2058

Merged
merged 2 commits into from
Jan 11, 2024

Conversation

TheSecondComing123
Copy link
Collaborator

No description provided.

Copy link
Member

@ysthakur ysthakur left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Just a tiny change (you can use the "Commit suggestion" button to apply it)

Comment on lines 1074 to 1075
"Zero Slice From | Collect While Unique | Complex Number",
List("zero-slice-from", "slice-from", "from", "drop-from", "collect-while-unique", "complex"),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Zero Slice From | Collect While Unique | Complex Number",
List("zero-slice-from", "slice-from", "from", "drop-from", "collect-while-unique", "complex"),
"Drop | Collect While Unique | Complex Number",
List("zero-slice-from", "slice-from", "drop", "collect-while-unique", "complex"),

The zero-slice-from and slice-from keywords are a good idea, but drop is also a common name for this operation (also, I've never seen anyone call it drop-from)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Personally, I've never heard of drop. I like the keywords, but maybe we could call it "Zero Slice From/Drop"?

Copy link
Member

@RubenVerg RubenVerg Jan 11, 2024

Choose a reason for hiding this comment

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

Drop is definitely the most common name, for example Scala, Haskell, and APL all call it "drop".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@RubenVerg Alright, I've changed it to Drop/Zero Slice From.

@TheSecondComing123 TheSecondComing123 added PR: Documentation Fix PR only tag - a PR that is updating/correcting/adding to documentation priority: medium Issues with medium priority difficulty: easy Very simple and won't take very long at all. PR: Enhancement PR only tag - for enhancements PR: Light and Easy PR Only Tag - Reviews here will probably be quick labels Jan 11, 2024
@TheSecondComing123 TheSecondComing123 changed the title rename drop to zero slice from rename drop to drop/zero slice from Jan 11, 2024
Copy link
Member

@ysthakur ysthakur left a comment

Choose a reason for hiding this comment

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

Calling it just Drop would've been too but I guess adding /Zero Slice From doesn't hurt. Nice job!

@ysthakur ysthakur merged commit f247c99 into Vyxal:version-3 Jan 11, 2024
3 checks passed
@TheSecondComing123
Copy link
Collaborator Author

Wait, there's still a problem. Some of the keywords aren't implemented in literate mode (e. g. zero-slice-from turns into zero-slice-|.

@ysthakur
Copy link
Member

What error are you getting? I tried locally and it's lexed correctly

@TheSecondComing123
Copy link
Collaborator Author

@ysthakur Oh woah it suddenly worked I must have tested it wrong.

@ysthakur
Copy link
Member

Awesome, good to hear

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: easy Very simple and won't take very long at all. PR: Documentation Fix PR only tag - a PR that is updating/correcting/adding to documentation PR: Enhancement PR only tag - for enhancements PR: Light and Easy PR Only Tag - Reviews here will probably be quick priority: medium Issues with medium priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants