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

Preserve user entries #6

Merged
merged 2 commits into from Feb 15, 2017

Conversation

Projects
None yet
2 participants
@haya14busa
Copy link
Contributor

haya14busa commented Feb 14, 2017

Preserve lines which is not placed between auto generated markers.

# Do not delete following entries.
user/entries/1
user/entries/2

# Delete lines between markers when updating.
# AUTO-GENERATED BY goimports-update-ignore
    genereated/by/goimports/1
    genereated/by/goimports/2
    ...
# END-AUTO-GENERATED BY goimports-update-ignore

# Do not delete following entries too.
user/entries/3
user/entries/4

Fixes #1

Preserve user entries
Preserve lines which is not placed between auto generated markers.

	# Do not delete following entries.
	user/entries/1
	user/entries/2

	# Delete lines between markers when updating.
	# AUTO-GENERATED BY goimports-update-ignore
        genereated/by/goimports/1
        genereated/by/goimports/2
        ...
	# END-AUTO-GENERATED BY goimports-update-ignore

	# Do not delete following entries too.
	user/entries/3
	user/entries/4

Fixes #1
@pwaller

This comment has been minimized.

Copy link
Owner

pwaller commented Feb 15, 2017

Thanks for the PR.

I just tested this, but it unfortunately doesn't recognise existing auto-generated files as auto-generated, so it causes the length of my configuration to double when I run it. This is not obvious because it takes the existing file and puts it at the top, which currently says that it's auto-generated at the top, and then appends the new auto-generated file.

A simple fix would be to simply avoid changing the comment line which is inserted. What do you think?

One other quick idea: where the user lists a manual rule, do not list it in the automatic section. That way the user doesn't have to comment out multiple lines if they happen to have both a manual rule and an automatic one. I will merge this PR without this change though, it's only if you want to do it.

@haya14busa

This comment has been minimized.

Copy link
Contributor Author

haya14busa commented Feb 15, 2017

A simple fix would be to simply avoid changing the comment line which is inserted. What do you think?

That's true. I fixed it.

One other quick idea: where the user lists a manual rule, do not list it in the automatic section. That way the user doesn't have to comment out multiple lines if they happen to have both a manual rule and an automatic one.

We don't need to implement this idea, i guess. Because if users added manual section, goimports-update-ignore takes care them when updating (goimports takes care it internally), so there should no duplicated section.

@pwaller

This comment has been minimized.

Copy link
Owner

pwaller commented Feb 15, 2017

Thanks. I've tested and that seems to fix the problem.

Because if users added manual section, goimports-update-ignore takes care them when updating (goimports takes care it internally), so there should no duplicated section.

I'm not sure we're understanding each other. My suggestion was to avoid duplicates appearing in the .goimportsignore file to avoid the confusion if a user wants to comment one out. To see this, move a line that appears in the auto-generated section outside into the manual section. Re-run goimports-update-ignore. It now appears twice, in both the manual section and outside. I think this can be confusing.

@haya14busa

This comment has been minimized.

Copy link
Contributor Author

haya14busa commented Feb 15, 2017

hmmm... that's true. I may misunderstood the goimports-update-ignore implementation somehow, i guess...

but, I think it's not so much important problem. if the directories is listed in generated section, it must be ignored regardless manual section contents. if go files are added to the directory, re-running goimports-update-ignore removes it from generated section and users can handle it for manual section.

I don't have strong motivation to implement the idea. I prefer creating another pull-request at least.
If you want to me to implement this, please let me know. Or could you please merging this pull-request?

@pwaller pwaller merged commit d2c92f7 into pwaller:master Feb 15, 2017

@haya14busa

This comment has been minimized.

Copy link
Contributor Author

haya14busa commented Feb 15, 2017

Thank you! 🐦 👍

@pwaller

This comment has been minimized.

Copy link
Owner

pwaller commented Feb 15, 2017

I don't mind too much, I don't think it is a big problem. Thanks again 👍.

@haya14busa haya14busa deleted the haya14busa:preserve-user-entries branch Feb 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.