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.

Sign up for GitHub

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

Jump to bottom

Clarify the internal module name #1412

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Clarify the internal module name #1412

wants to merge 2 commits into from

Conversation

epage
Copy link
Contributor

@epage epage commented Sep 28, 2021

As part of #1410, the name internal.rs was discussed and it was
brought up that parser.rs would provide clearer intent.

@epage epage changed the title Clarify the internal module name Clarify the internal module name Sep 28, 2021
@epage
Copy link
Contributor Author

epage commented Sep 30, 2021

@Stargateur what would be the path for moving this and #1413 forward so I can work on the issues I've created?

@Stargateur
Copy link
Contributor

@Stargateur what would be the path for moving this and #1413 forward so I can work on the issues I've created?

I have no control on this repo, gael have not much time so... wait until gael have time ? Thus I considering ask gael to be a maintainer of nom.

@epage
Copy link
Contributor Author

epage commented Oct 1, 2021

Ok, didn't realize he was the only one with merge permission.

@Geal
Copy link
Collaborator

Geal commented Oct 10, 2021

parser is a better name than internal, yes. This will have to wait for the 8.0 nom though, I'm opening the milestone for it

@Geal Geal added this to the 8.0 milestone Oct 10, 2021
@epage
Copy link
Contributor Author

epage commented Oct 11, 2021

parser is a better name than internal, yes. This will have to wait for the 8.0 nom though, I'm opening the milestone for it

How come this has to wait for nom 8? The internal mod isn't exposed, so this shouldn't be a breaking change.

Xiretza
Xiretza suggested changes Jan 14, 2022
View reviewed changes
Copy link
Contributor

@Xiretza Xiretza left a comment

Choose a reason for hiding this comment

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

Apart from the rustfmt noise, this LGTM, and I agree that this is not an externally visible change and can thus be merged immediately.

src/lib.rs Show resolved Hide resolved
@epage epage force-pushed the rename branch 2 times, most recently from 207e4b5 to 4595bf4 Compare January 14, 2022 18:22
@epage
refactor: Clarify intent of internal module
7f81e76 
As part of rust-bakery#1410, the name `internal.rs` was discussed and it was
brought up that `parser.rs` would provide clearer intent.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 79.556% when pulling 7f81e76 on epage:rename into e99f9e0 on Geal:main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xiretza Xiretza Xiretza approved these changes

At least 0 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
8.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@epage @Stargateur @Geal @coveralls @Xiretza