Skip to content

Minor fix after version 0.11.0 #209

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

Merged
merged 10 commits into from
Dec 31, 2020
Merged

Minor fix after version 0.11.0 #209

merged 10 commits into from
Dec 31, 2020

Conversation

jcs090218
Copy link
Collaborator

This patch does the following.

  • Remove trailing spaces
  • Avoid following warnings while installing
In csharp-tree-sitter-mode:
csharp-tree-sitter.el:270:9:Warning: assignment to free variable
    ‘csharp-mode-syntax-table’
csharp-tree-sitter.el:271:9:Warning: assignment to free variable
    ‘csharp-mode-map’

@theothornhill
Copy link
Collaborator

It seems like it fails on tree-sitter-indent for some reason. Not sure why. I thought it was supported on 26.1. I'll look into this a little.

Can you clean up commit history a little? Maybe 3 commits, one for each logical change?

  • whitespace
  • requiring and package dependencies
  • with-eval-after-load

Otherwise, I have a few comments :) Thanks for the pr!

@jcs090218
Copy link
Collaborator Author

It seems like it fails on tree-sitter-indent for some reason. Not sure why. I thought it was supported on 26.1. I'll look into this a little.

Yes, please! I don't know why CI is also reporting the seq is invalid.

Can you clean up commit history a little? Maybe 3 commits, one for each logical change?

Sure. Thanks for the review!

@theothornhill
Copy link
Collaborator

Build is green! Yay :) It is transitive dependencies with tree-sitter-indent that is causing these errors. Without doing lots of research, it might be because 2.21 is too new for emacs 26.1, only released with 26.3, which don't get that error as far as I can tell. I'll check with them at some point.

Did we address your issues well enough with these changes?

@jcs090218
Copy link
Collaborator Author

jcs090218 commented Dec 31, 2020

Did we address your issues well enough with these changes?

Yes! I think this suppose to address everything from what I am seeing. :)

I might need to create another PR for git history cleaning. I don't know how I am suppose to clean up in the current branch. It's a bit too messy now. 😕

@theothornhill
Copy link
Collaborator

Did we address your issues well enough with these changes?

Yes! I think this suppose to address everything from what I am seeing. :)

Great!

I might need to create another PR for git history cleaning. I don't know how I am suppose to clean up in the current branch. It's a bit too messy now. 😕

No worries, I'll just squash it, since it is a little smaller now :) Thanks so much for this 👍

@theothornhill theothornhill merged commit 91d5161 into master Dec 31, 2020
@jcs090218
Copy link
Collaborator Author

No worries, I'll just squash it, since it is a little smaller now :) Thanks so much for this

Thanks for the kindly review! And a being very fast to address issue! ;)

By the way, I wonder who is in the organization (since there are no public members)? Can I joined the organization and I would like help maintain a little to this amazing package! 😃 I will certainly help when I have time! Thanks!

@jcs090218 jcs090218 deleted the minor branch December 31, 2020 08:46
@josteink
Copy link
Collaborator

By the way, I wonder who is in the organization (since there are no public members)?

I believe its a 2-person organisation for now. Me and @theothornhill.

Can I joined the organization and I would like help maintain a little to this amazing package! 😃 I will certainly help when I have time! Thanks!

More contributors is a good thing.

If you keep on sending in PRs regularly we'll surely add you to the list 🙂

This was referenced Dec 31, 2020
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