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

Load gitignore file #1273

Merged

Conversation

kentarolim10
Copy link
Contributor

@kentarolim10 kentarolim10 commented Oct 6, 2023

Description

-Throws a 403 if the user tries to open a .gitignore file but the file is hidden
-Displays a helpful message showing how to enable hidden files
-Adds checkbox to not show helpful message in the feature
-Allow user to open, view and edit the .gitignore file

Temporary fix for the opening the .gitignore file in #1168

@welcome
Copy link

welcome bot commented Oct 6, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@github-actions
Copy link

github-actions bot commented Oct 6, 2023

Binder 👈 Launch a Binder on branch kentarolim10/jupyterlab-git/gitignore-wont-open-if-file-hidden

@kentarolim10 kentarolim10 changed the title [Draft] Load gitignore file Load gitignore file Oct 11, 2023
@fcollonval fcollonval changed the base branch from main to jlab-3 October 24, 2023 13:31
@fcollonval
Copy link
Member

@kentarolim10 the main branch has been updated to JupyterLab 4 - to ease the work here, I changed the target to the branch jlab-3 to avoid complex rebasing.

@kentarolim10
Copy link
Contributor Author

kentarolim10 commented Oct 24, 2023 via email

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @kentarolim10

This looks really good. I have only one large concern about the detection if .gitignore is accessible and how to get its content; we should try to split the code in more limited feature. Let me know if my proposal for improvement is not clear.

"""
try:
file = pathlib.Path(path)
if file.stat().st_size > 0:
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason for testing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wanted to check if the file was empty and then return an empty string if it was. I guess since if the file is empty, then it would be an empty string regardless. I will remove it.

@@ -1681,6 +1681,22 @@ async def remote_remove(self, path, name):

return response

async def read_file(self, path):
Copy link
Member

Choose a reason for hiding this comment

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

This is actually not asynchronous 😉

Suggested change
async def read_file(self, path):
def read_file(self, path):

@@ -1721,6 +1737,32 @@ async def ignore(self, path, file_path):
return {"code": -1, "message": str(error)}
return {"code": 0}

async def writeGitignore(self, path, content):
Copy link
Member

Choose a reason for hiding this comment

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

Let's be consistent with the naming of Python (even if I find myself often bitten by this one too):

Suggested change
async def writeGitignore(self, path, content):
async def write_gitignore(self, path, content):

Comment on lines 1756 to 1761
with gitignore.open("a") as f:
f.truncate(0)
f.seek(0)
f.write(content)
if content[-1] != "\n":
f.write("\n")
Copy link
Member

Choose a reason for hiding this comment

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

You could write directly the file content:

Suggested change
with gitignore.open("a") as f:
f.truncate(0)
f.seek(0)
f.write(content)
if content[-1] != "\n":
f.write("\n")
if content and content[-1] != "\n":
content += "\n"
gitignore.write_text(content)

See the doc for more info: https://docs.python.org/3/library/pathlib.html#pathlib.Path.write_text

editor.disposed.connect(() => {
model.dispose();
});
const preview = new PreviewMainAreaWidget({
Copy link
Member

Choose a reason for hiding this comment

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

Let's use a MainAreaWidget directly here as the intention is to edit the file content:

Suggested change
const preview = new PreviewMainAreaWidget({
const preview = new MainAreaWidget({

src/git.ts Outdated
Comment on lines 68 to 70
if (response.status === 403) {
throw new Git.HiddenFile(data.content);
}
Copy link
Member

Choose a reason for hiding this comment

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

We unfortunately gonna need to change the logic because this is not robust (we may get a 403 code for other reasons).

Comment on lines 421 to 423
await showGitignore(error);
} else {
await showGitignoreHiddenFile(error);
Copy link
Member

Choose a reason for hiding this comment

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

Could you change a bit the signature of showGitignoreHiddenFile to take a second optional argument that will decide if the user should be prompted with a dialog or not? showGitignore will therefore be an implementation detail of showGitignoreHiddenFile; the goal is to reduce the API between the various part of the code.

Comment on lines 833 to 835
if not self.serverapp.contents_manager.allow_hidden and not content:
self.set_status(403, "hidden files cannot be accessed")
body["content"] = await self.git.read_file(local_path + "/.gitignore")
Copy link
Member

Choose a reason for hiding this comment

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

We need to change the logic because that code is increasing the complexity of the endpoint and returns the file content in any case even if the user wants to respect the configuration of not accessing the hidden file.

src/model.ts Show resolved Hide resolved
src/tokens.ts Outdated
Comment on lines 1304 to 1309
content: string;
constructor(content: string) {
super('File is hidden');
this.name = 'hiddenFile';
this.message = 'File is hidden and cannot be accessed.';
this.content = content;
Copy link
Member

Choose a reason for hiding this comment

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

content will not be needed

Suggested change
content: string;
constructor(content: string) {
super('File is hidden');
this.name = 'hiddenFile';
this.message = 'File is hidden and cannot be accessed.';
this.content = content;
constructor(content: string) {
super('File is hidden');
this.name = 'hiddenFile';
this.message = 'File is hidden and cannot be accessed.';

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Almost done; would you mind addressing the new comment and that one?

src/model.ts Outdated
Comment on lines 891 to 899
async readGitIgnore(): Promise<{ code: number; content: string }> {
const path = await this._getPathRepository();

const res = (await requestAPI(URLExt.join(path, 'ignore'), 'GET')) as {
code: number;
content: string;
};
await this.refreshStatus();
return res;
Copy link
Member

Choose a reason for hiding this comment

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

Let's make the method more useful by returning directly the content. And we don't need to refresh the status as it does not change the gitignore file.

Suggested change
async readGitIgnore(): Promise<{ code: number; content: string }> {
const path = await this._getPathRepository();
const res = (await requestAPI(URLExt.join(path, 'ignore'), 'GET')) as {
code: number;
content: string;
};
await this.refreshStatus();
return res;
async readGitIgnore(): Promise<string> {
const path = await this._getPathRepository();
return (await requestAPI(URLExt.join(path, 'ignore'), 'GET')).content;

@fcollonval
Copy link
Member

almost there @kentarolim10

You only need to fix (it is the consequence of the API returning now directly the gitignore content):

ERROR: src/commandsAndMenu.tsx:329:45 - error TS2339: Property 'content' does not exist on type 'string'.

329     model.sharedModel.setSource(contentData.content ? contentData.content : '');
                                                ~~~~~~~
src/commandsAndMenu.tsx:329:67 - error TS2339: Property 'content' does not exist on type 'string'.

329     model.sharedModel.setSource(contentData.content ? contentData.content : '');

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks a lot @kentarolim10

Let's merge 😉

@fcollonval fcollonval merged commit 23fa764 into jupyterlab:jlab-3 Nov 15, 2023
8 checks passed
Copy link

welcome bot commented Nov 15, 2023

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@fcollonval
Copy link
Member

@meeseeksdev please backport to main

Copy link

lumberbot-app bot commented Nov 15, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout main
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 23fa7648df8ad1b01c5ddae7d4db62b1b9f15337
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #1273: Load gitignore file'
  1. Push to a named branch:
git push YOURFORK main:auto-backport-of-pr-1273-on-main
  1. Create a PR against branch main, I would have named this PR:

"Backport PR #1273 on branch main (Load gitignore file)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@fcollonval
Copy link
Member

@all-contributors please add @kentarolim10 for code

Copy link
Contributor

@fcollonval

I've put up a pull request to add @kentarolim10! 🎉

fcollonval pushed a commit to fcollonval/jupyterlab-git that referenced this pull request Nov 16, 2023
* Throw 403 if file is hidden

* Add Helpful GUI message for hidden files

* Fetch file contents if hidden file

* Show hidden file in widget

* Format gitignore file

* Show hidden file message when adding file to .gitignore

* Add ability to save .gitignore

* Fix bug where you can open two .gitignore files

* Add option to hide hidden file warning

* Fix PR requests for gitignore bug

* Fix prettier styles for gitignore

* Improve translation

* Improve gitignore model and add hiddenFile option to schema

* Fix eslint

* Fix .gitignore content sending

---------

Co-authored-by: Frédéric Collonval <[email protected]>
(cherry picked from commit 23fa764)
fcollonval added a commit that referenced this pull request Nov 16, 2023
* Load gitignore file (#1273)

* Throw 403 if file is hidden

* Add Helpful GUI message for hidden files

* Fetch file contents if hidden file

* Show hidden file in widget

* Format gitignore file

* Show hidden file message when adding file to .gitignore

* Add ability to save .gitignore

* Fix bug where you can open two .gitignore files

* Add option to hide hidden file warning

* Fix PR requests for gitignore bug

* Fix prettier styles for gitignore

* Improve translation

* Improve gitignore model and add hiddenFile option to schema

* Fix eslint

* Fix .gitignore content sending

---------

Co-authored-by: Frédéric Collonval <[email protected]>
(cherry picked from commit 23fa764)

* Fix tests

---------

Co-authored-by: Kentaro Lim <[email protected]>
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.

2 participants