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

Auto-linking - cm_autolinks test #1092

Closed
joshbruce opened this issue Feb 27, 2018 · 10 comments · Fixed by #1135
Closed

Auto-linking - cm_autolinks test #1092

joshbruce opened this issue Feb 27, 2018 · 10 comments · Fixed by #1135

Comments

@joshbruce
Copy link
Member

Marked version: 0.3.17

Proposal type: other

What pain point are you perceiving?

Travis CI is still in untrustworthy state because we have a failing test: cm_autolinks

I don't think auto-linking is a feature from a specification is it? If not, is it something marked took on at some point and we're trying to keep alive for now to maintain backward compatibility?

I don't know about the rest of y'all but I keep getting emails from Travis saying, "Build X is still failing".

What solution are you suggesting?

  1. How are we on fixing autolinking?
  2. Should we fix autolinking and why?
  3. Can/Should we remove the test for now until it gets fixed - create an issue so we know it's something we need to do?
@styfle
Copy link
Member

styfle commented Feb 28, 2018

@intcreator
Copy link

Failing tests kind of drive me insane, so I'd like to investigate this a bit further. Feder1co5oave mentioned that he had a branch that addressed this issue in a comment, but the actual branch is a personal fork. My questions are:

  1. Is there enough value to justify salvaging this branch?
  2. If so, what additional tests need to be added (per Feder1co5oave's comment)
  3. If not, is there an easy alternative fix?

Here is the diff of Feder1co5oave's branch with Marked master.

@joshbruce
Copy link
Member Author

  1. I think so?? (He does really great work.)
  2. Which comment, not sure I'm following??
  3. I do not know and not sure who to tag.

Also, updated good first issues with the consolidation issues Feder1co5oave was, for better or worse, taking on almost entirely by himself. See #984, #1036, and #1043 for how our process and interactions were working there.

Once we get the architecture stuff a little bit more figured out from #1096, I might be able to help with straight refactoring (things that only improve readability) code submissions; thereby, not breaking or trying to fix anything before I actually know what's going on better.

@intcreator
Copy link

  1. Cool let's do it.
  2. It's the same comment that I linked in the description
  3. Let's focus on the branch then

I'll take a look at some point and see if I can figure out what Feder1co5oave's intent was, then look into additional tests.

@joshbruce
Copy link
Member Author

  1. Right on.
  2. Doh! Sorry, blind moment.
  3. Sounds good.

Sounds good. I might need to step away briefly to catch up on some other things, but I'm starting to think of ways we might be able to do some simple refactoring just to make the code easier to read and digest.

(A lot of switching contexts as I read it top-down, thinking reorganizing what's there, and comments to help with migrating to 1.0+ - comments would add bloat, but I think most users would use the min client-side and the CLI is running locally; so, shouldn't hurt performance on that score.)

@joshbruce
Copy link
Member Author

joshbruce commented Feb 28, 2018

ps. Are there JS versions that actually care about code ordering - haven't seen a language in a long time where that mattered??

@intcreator
Copy link

We won't be doing too much refactoring before 1.0 though, right? I think comments/labeling are okay for 1.0 but structural changes should probably wait (according to the current sentiment I'm sensing toward 1.0).

What exactly do you mean by code ordering? Are you referring to function hoisting?

@joshbruce
Copy link
Member Author

Ah. That's fair.

I distinguish between refactoring and re-engineering.

Refactoring (defined by the book):

“Refactoring is the process of changing a software system in such a way that it does not alter the external behavior of the code yet improves its internal structure.” ~ “Refactoring: Improving the Design of Existing Code.” iBooks

So, we change the way the code reads, but not how people interact with it, what it does, or anything else (like editing a term paper without changing the actual subject or message). Therefore, changes made should not result in a breaking change or need to write new tests (I tend to come from the camp that tests aren't there to validate a system, they're there to make sure I didn't break anything when I refactor and to document how the code works for other developers - replacement of technical specifications and other related documentation).

Re-engineering:

"Software Re-engineering is the examination and alteration of a system to reconstitute it in a new form." decent Slideshare?? where I got the definition - first result in Google; so, totally adding the disclaimer. 😉

@joshbruce
Copy link
Member Author

What I did to the README last week was re-engineering; however, most of the content remained. If I would have just edited the content without breaking up the README into all the other files, that would have been refactoring.

@joshbruce
Copy link
Member Author

joshbruce commented Feb 28, 2018

ps. For me.

If we trust our tests as a good safety net - we can make that marked.js read any way we want - without actually changing anything functionally. (Check out that book - teaches the methods of manually performing refactoring - how I normally do it. Or this video - he has some other talks where he talks about malleability of code - in this one he's using his IDE to actually perform the refactoring.)

If we don't trust the tests - then we probably don't understand the code [or requirements] well enough to confirm (by other means) that we haven't broken things. When we find ways to test something (fix a discovered defect) - we should write a test...not to go all TDD or anything on you. 😆

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

Successfully merging a pull request may close this issue.

3 participants