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

Add list view and a markdown readme #3

Closed
wants to merge 12 commits into from
Closed

Conversation

@bueltge
Copy link

@bueltge bueltge commented Dec 1, 2015

Hi Shaun,

I use your url-shortener since a lot of years. Currently I need a list view and I enhance your script.
Also I add a readme in markdown format to read it much better on github.
Maybe you enjoy the enhancement.
Best.

index.php Outdated

define('COOKIE_NAME', DB_PREFIX.'auth');
define('COOKIE_VALUE', md5(USERNAME.PASSWORD.COOKIE_SALT));
define('COOKIE_DOMAIN', '.'.LESSN_DOMAIN);

This comment has been minimized.

@jeffbyrnes

jeffbyrnes Dec 1, 2015

The addition of all of these constants is curious; how does this help with the list view? Is it truly necessary, or something that ended up in here that is related, but separate?

@jeffbyrnes
Copy link

@jeffbyrnes jeffbyrnes commented Dec 1, 2015

👍 on the addition of a list view, though some stuff in there seems unnecessary to me (probably just don’t fully understand the context or usage).

This can probably delete README.txt to avoid any confusion.

@bueltge
Copy link
Author

@bueltge bueltge commented Dec 1, 2015

@jeffbyrnes Thanks for the helping comments. I have centralized the constants. My idea was currently, not to much changes in the original source. Also now I have add a update readme and remove the readme.txt, replaced by the md-file. Maybe now is it more clear.

@jeffbyrnes
Copy link

@jeffbyrnes jeffbyrnes commented Dec 1, 2015

@bueltge you’re welcome. Thanks for the explanation w/r/t the constants; perhaps an interactive rebase & some additional detail in the commit message would be helpful for the long term?

@bueltge
Copy link
Author

@bueltge bueltge commented Dec 1, 2015

Thanks. Which long term do you mean? Is a commit message from my side not clear enough, to much changes or wrong, not understandable commit message?

@jeffbyrnes
Copy link

@jeffbyrnes jeffbyrnes commented Dec 1, 2015

@bueltge the commit message itself is what I meant; that could say more about what your intent is, and why, you did it that way.

Definitely like how you now have moved all the constants to their own, single file. Much better to have them just once!

Are you familiar with interactive rebasing?

@bueltge
Copy link
Author

@bueltge bueltge commented Dec 1, 2015

No, currently I'm not with the interactive rebasing. Now I use seldom this git commit --amend, not more. Maybe you have a hint, link for me?
I think, I understand you goal, that the commit is clear for all readers there have interest, not to short and without details about why.

@jeffbyrnes
Copy link

@jeffbyrnes jeffbyrnes commented Dec 1, 2015

@bueltge exactly that, in fact. Two resources to learn about interactive rebasing:

It’s a tricky concept. To help you out, I went ahead & did a bit of rebasing of your commits myself. I’ll open another PR for this exact same work, and you can compare & use it as an example.

@jeffbyrnes
Copy link

@jeffbyrnes jeffbyrnes commented Dec 1, 2015

And there we are. Please feel free to take a look! All credit to you, I just moved things around and cleaned things up a bit, expanded on the commit messages, and broke the version bump out to its own commit.

An aside to that, in most projects it’s considered best practice to let the maintainer handle changing the version number, updating the docs, and releasing said new version through whatever means they use. That said, here, it seems alright.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.